On Thu, Apr 04, 2019 at 02:40:19PM +0200, Raphael Graf wrote:
> The attached diff adds input support to portmidi.
> For each open input device, a thread is started, waiting for input using
> poll(2).
>
> The following program can be used for testing input and ouput:
> /usr/ports/pobj/portmidi-217/build-amd64/Release/test
> (it is not installed by the port)
>
> If I understand the API correctly, portmidi is not a good choice for realtime
> midi-input. There is no callback functionality for reading, so busy-waiting is
> needed, like this:
>
> while (Pm_Poll(midi)) {
> Pm_Read(midi, buffer, 1);
> }
>
Thank you very much for your update.
I've never used portmidi, but this lack of multiplexing forces the
caller to also start a thread (per device) and we'd end up with two
threads per device!
> (see http://portmedia.sourceforge.net/portmidi/doxygen/group__grp__io.html)
>
> Currently, audacity is the only user of portmidi, only using it for output.
> .. so I'm not sure how useful this diff is ;)
I remember having tried few times to download & build midi programs
until I notice they depend on portmidi. Now I can try again :-)
>
> Any comments or ok?
>
few comments about the midi bits below:
> Index: files/pm_sndio/pmsndio.c
> ===================================================================
> RCS file: /cvs/ports/audio/portmidi/files/pm_sndio/pmsndio.c,v
> retrieving revision 1.1.1.1
> diff -u -p -u -p -r1.1.1.1 pmsndio.c
> --- files/pm_sndio/pmsndio.c 23 Mar 2019 13:30:08 -0000 1.1.1.1
> +++ files/pm_sndio/pmsndio.c 4 Apr 2019 11:03:08 -0000
> @@ -3,9 +3,13 @@
> #include <stdlib.h>
> #include <stdio.h>
> #include <sndio.h>
> +#include <string.h>
> +#include <poll.h>
> +#include <pthread.h>
> #include "portmidi.h"
> #include "pmutil.h"
> #include "pminternal.h"
> +#include "porttime.h"
>
>
> PmDeviceID pm_default_input_device_id = -1;
> @@ -14,28 +18,70 @@ PmDeviceID pm_default_output_device_id =
> extern pm_fns_node pm_sndio_in_dictionary;
> extern pm_fns_node pm_sndio_out_dictionary;
>
> +#define NDEVS 9
> +#define SYSEX_MAXLEN 256
> +
> +struct mio_dev {
> + char name[16];
> + struct mio_hdl *hdl;
> + int mode;
> + char errmsg[PM_HOST_ERROR_MSG_LEN];
> + pthread_t thread;
> +} devs[NDEVS];
> +
> +static void set_mode(struct mio_dev *, unsigned int);
> +
> void pm_init()
> {
> - // Add output devices
> - pm_add_device("SNDIO",
> - "default",
> - FALSE,
> - (void *)0,
> - &pm_sndio_out_dictionary);
> - pm_add_device("SNDIO",
> - "midi/0",
> - FALSE,
> - (void *)1,
> - &pm_sndio_out_dictionary);
> + int i, j = 0;
> +
> + /* default */
> + strcpy(devs[j].name, MIO_PORTANY);
> + pm_add_device("SNDIO", devs[j].name, TRUE, (void *) &devs[j],
> + &pm_sndio_in_dictionary);
> + pm_add_device("SNDIO", devs[j].name, FALSE, (void *) &devs[j],
> + &pm_sndio_out_dictionary);
> + j++;
> +
> + /* midithru */
> + for (i = 0; i < 4; i++) {
> + sprintf(devs[j].name, "midithru/%d", i);
> + pm_add_device("SNDIO", devs[j].name, TRUE, (void *) &devs[j],
> + &pm_sndio_in_dictionary);
> + pm_add_device("SNDIO", devs[j].name, FALSE, (void *) &devs[j],
> + &pm_sndio_out_dictionary);
> + j++;
> + }
> +
> + /* rmidi */
> + for (i = 0; i < 4; i++) {
> + sprintf(devs[j].name, "rmidi/%d", i);
> + pm_add_device("SNDIO", devs[j].name, TRUE, (void *) &devs[j],
> + &pm_sndio_in_dictionary);
> + pm_add_device("SNDIO", devs[j].name, FALSE, (void *) &devs[j],
> + &pm_sndio_out_dictionary);
> + j++;
> + }
>
I'd also add few "midi/%d" and "snd/%d" devices, the latter are are
MIDI as well (input gets notification about volume changes and the
sound card wall clock).
But I wonder if it makes sense to try to enumerate all possible ones.
The "default" device can handle virtually anything as the user can run
the proper midicat instances to handle as many devices it wishes. At
least that's how I proceed.
> +void* input_thread(void *param)
> +{
> + PmInternal *midi = (PmInternal*)param;
> + struct mio_dev *dev = (struct mio_dev *) midi->descriptor;
> + struct pollfd pfd[1];
> + nfds_t nfds;
> + unsigned char status, byte = 0;
> + int count = 0, msglen;
> + PmEvent pm_ev, pm_ev_rt;
> + unsigned char sysex_data[SYSEX_MAXLEN];
> +
> + while(dev->mode & MIO_IN) {
> + nfds = mio_pollfd(dev->hdl, pfd, POLLIN);
> + if (poll(pfd, nfds, 100) < 0) {
poll could return with EINTR errno, which is legitimate (ex, if the
program is using timers or catches signals). If so poll() must be
restarted (unless dev->mode chaged).
> + fprintf(stderr, "poll error; aborting midi thread\n");
> + break;
> + }
> + if (!(mio_revents(dev->hdl, pfd) & POLLIN))
> + continue;
> +
Reading a single byte means one syscall per byte, it might have an
impact on the system. Another option is to use a buffer and refill it
when needed:
unsigned char buf[0x200], *p;
size_t todo = 0;
int revents;
while (1) {
if (todo == 0) {
nfds = mio_pollfd(..., POLLIN);
rc = poll(...);
if (rc < 0) {
if (errno == EINTR)
continue;
break;
}
revents = mio_revents(...);
if (revents & POLLHUP)
break;
if (!(revents & POLLIN))
continue;
todo = mio_read(hdl, buf, sizeof(buf));
if (todo == 0)
continue;
p = buf;
}
byte = *p++;
todo--;
/* parse *p here */
}
> + if (!mio_read(dev->hdl, &byte, 1)) {
> + fprintf(stderr, "read error; aborting midi thread\n");
> + break;
> + }
> +
> + if (byte & 0x80) {
> + if (byte >= 0xf8) {
> + /* realtime message */
> + pm_ev_rt.message = byte;
> + pm_ev_rt.timestamp = Pt_Time();
> + pm_read_short(midi, &pm_ev_rt);
> + continue;
> + }
> + status = byte;
> + switch(byte) {
> + case 0xf0: /* sysex */
> + sysex_data[0] = byte;
> + msglen = 0;
> + count = 0;
as this first byte entered the buffer, shouldn't count start at 1?
> + break;
> + case 0xf7: /* sysex end */
> + sysex_data[count] = byte;
same question here, shouldn't count be incremented here as well?
> + break;
> + default: /*status */
Handling of common message 0xf1..0xf6 is missing, they are different
from 0x80..0xef, as they reset the status byte.
> + pm_ev.message = byte;
> + pm_ev.timestamp = Pt_Time();
> + msglen = midi_message_length(pm_ev.message);
> + count = 0;
> + break;
> + }
we may receive data without status byte. An easy way to handle this
is to initialize status = 0 and below do:
} else if (status != 0) {
if (status == 0xf0)
...
...
> + } else { /* data */
> + if (status == 0xf0) /* sysex data */
> + if (count < SYSEX_MAXLEN - 1) {
> + sysex_data[count] = byte;
> + } else {
> + fprintf(stderr, "the message is too long\n");
> + continue;
> + }
> + else if (count > 0) /* short messge */
> + pm_ev.message |= (byte << (8 * count));
> + }
> +
> + count++;
> +
> + if (status == 0xf7) /* sysex data received */
> + pm_read_bytes(midi, &sysex_data[0], count, Pt_Time());
> + else if (count == msglen) /* short message received */
> + pm_read_short(midi, &pm_ev);
> + }
For this part, you could take a look to the midi_in() function in
src/usr.bin/sndiod/midi.c, it does exactly the same thing but with
different variable names.
> + pthread_exit(NULL);
> + return NULL;
> +}
> +
> +static void set_mode(struct mio_dev *dev, unsigned int mode) {
> + if (dev->mode != 0)
> + mio_close(dev->hdl);
> + dev->mode = 0;
> + if (mode != 0)
> + dev->hdl = mio_open(dev->name, mode, 1);
I'd suggest not using the non-blocking mode, because Pm_Write() is
blocking and poll(4) ensures we never block in mio_read().
> + if (dev->hdl)
> + dev->mode = mode;
> +}
> +
> static PmError sndio_out_open(PmInternal *midi, void *driverInfo)
> {
> - const char *device = descriptors[midi->device_id].pub.name;
> - struct mio_hdl *mio;
> + descriptor_type desc = &descriptors[midi->device_id];
> + struct mio_dev *dev = (struct mio_dev *) desc->descriptor;
>
> - mio = mio_open(device, MIO_OUT, 0);
> - if (!mio) {
> - fprintf(stderr, "mio_open failed\n");
> + if (dev->mode & MIO_OUT)
> + return pmNoError;
> +
> + set_mode(dev, dev->mode | MIO_OUT);
> + if (!(dev->mode & MIO_OUT)) {
> + snprintf(dev->errmsg, PM_HOST_ERROR_MSG_LEN,
> + "mio_open (output) failed: %s\n", dev->name);
> return pmHostError;
> }
> - midi->descriptor = mio;
>
> + midi->descriptor = (void *)dev;
> return pmNoError;
> }
> +
> +static PmError sndio_in_open(PmInternal *midi, void *driverInfo)
> +{
> + descriptor_type desc = &descriptors[midi->device_id];
> + struct mio_dev *dev = (struct mio_dev *) desc->descriptor;
> +
> + if (dev->mode & MIO_IN)
> + return pmNoError;
> +
> + set_mode(dev, dev->mode | MIO_IN);
> + if (!(dev->mode & MIO_IN)) {
> + snprintf(dev->errmsg, PM_HOST_ERROR_MSG_LEN,
> + "mio_open (input) failed: %s\n", dev->name);
> + return pmHostError;
> + }
> + midi->descriptor = (void *)dev;
> + pthread_attr_t attr;
> + pthread_attr_init(&attr);
> + pthread_create(&dev->thread, &attr, input_thread, ( void* )midi);
> + return pmNoError;
> +}
> +
> static PmError sndio_out_close(PmInternal *midi)
> {
> - mio_close(midi->descriptor);
> + struct mio_dev *dev = (struct mio_dev *) midi->descriptor;
> +
> + if (dev->mode & MIO_OUT)
> + set_mode(dev, dev->mode & ~MIO_OUT);
> + return pmNoError;
> +}
> +
> +static PmError sndio_in_close(PmInternal *midi)
> +{
> + struct mio_dev *dev = (struct mio_dev *) midi->descriptor;
> +
> + if (dev->mode & MIO_IN) {
> + set_mode(dev, dev->mode & ~MIO_IN);
> + pthread_join(dev->thread, NULL);
> + dev->thread = NULL;
> + }
> return pmNoError;
> }
> +
> static PmError sndio_abort(PmInternal *midi)
> {
> - mio_close(midi->descriptor);
> return pmNoError;
> }
> +
> static PmTimestamp sndio_synchronize(PmInternal *midi)
> {
> return 0;
> }
> -static PmError sndio_write_byte(PmInternal *midi, unsigned char byte,
> - PmTimestamp timestamp)
> +
> +static PmError do_write(struct mio_dev *dev, const void *addr, size_t nbytes)
> {
> - size_t w = mio_write(midi->descriptor, &byte, 1);
> - if (w != 1) {
> - fprintf(stderr, "mio_write failed\n");
> + int nfds, revents;
> + struct pollfd pfds[1];
> +
> + do {
> + nfds = mio_pollfd(dev->hdl, pfds, POLLOUT);
> + if (poll(pfds, nfds, -1) < 0) {
> + fprintf(stderr, "poll error; write failed\n");
> + break;
> + }
> + revents = mio_revents(dev->hdl, pfds);
> + } while (!(revents & POLLOUT));
> +
> + size_t w = mio_write(dev->hdl, addr, nbytes);
If using non-blocking mio_write(), then mio_write() must wrapped in a
loop to handle short writes. Ex:
unsigned char *data = addr;
size_t todo = nbytes;
while (todo > 0) {
nfds = mio_pollfd(..., POLLOUT);
poll(...)
revents = mio_revents(...);
if (revents & POLLHUP)
return pmHostError;
if (revents & POLLOUT) {
w = mio_write(hdl, data, todo);
todo -= w;
data += w;
}
}
but this is equivalent to a blocking write, so it's much easier to
just use the blocking mode.
HTH
-- Alexandre
