[PATCH 1/1] handling sysex midi messages in winecoreaudio

[Andrew Eikum]

On Wed, Feb 25, 2015 at 09:58:41PM +0100, tinez wrote:
> Hi,
> 
> this patch fixes handling sysex midi messages in winecoreaudio
> driver. The code was based on winealsa driver implementation.
> I've tested it using external midi hardware + midi monitoring tools
> (MidiOX). My motivation was to fix Nord Modular Editor software
> which is needed to control Nord Modular G1 hardware synthetizer.
> After applying the patch it works without problems.
> I've also sent out patch to electro-music.com for other people to
> test and it was received well (you can see the thread here
> electro-music.com/forum/viewtopic.php?t=62683).
> 

Thanks, looks mostly good. A few Wine style notes in-line below.

> regards, tinez
> 
> ---
>  dlls/winecoreaudio.drv/midi.c | 66
> ++++++++++++++++++++++++++++++-------------
>  1 file changed, 47 insertions(+), 19 deletions(-)

> From e18b1c9876730d9557b97f3e16249b20eae4e875 Mon Sep 17 00:00:00 2001
> From: tinez <tinez at tlen.pl>

Wine requires full, real names for patches.

> Date: Thu, 21 Aug 2014 23:13:31 +0200
> Subject: [PATCH 1/1] handling sysex midi messages in winecoreaudio
> 
> ---
>  dlls/winecoreaudio.drv/midi.c | 66 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 47 insertions(+), 19 deletions(-)
> 
> diff --git a/dlls/winecoreaudio.drv/midi.c b/dlls/winecoreaudio.drv/midi.c
> index 8593e9c..c7a1ded 100644
> --- a/dlls/winecoreaudio.drv/midi.c
> +++ b/dlls/winecoreaudio.drv/midi.c
> @@ -437,9 +437,9 @@ static DWORD MIDIOut_LongData(WORD wDevID, LPMIDIHDR lpMidiHdr, DWORD dwSize)
>              return MMSYSERR_ERROR;
>          }
>      }
> -    else
> +    else if (destinations[wDevID].caps.wTechnology == MOD_MIDIPORT)
>      {
> -        FIXME("MOD_MIDIPORT\n");
> +        MIDIOut_Send(MIDIOutPort, destinations[wDevID].dest, lpData, lpMidiHdr->dwBufferLength);
>      }
>  
>      lpMidiHdr->dwFlags &= ~MHDR_INQUEUE;
> @@ -836,26 +836,55 @@ static CFDataRef MIDIIn_MessageHandler(CFMessagePortRef local, SInt32 msgid, CFD
>              if (src->state < 1)
>              {
>                  TRACE("input not started, thrown away\n");
> -                goto done;
> -            }
> -            /* FIXME skipping SysEx */
> -            if (msg->data[0] == 0xF0)
> -            {
> -                FIXME("Starting System Exclusive\n");
> -                src->state |= 2;
> +                return NULL;
>              }
> -            if (src->state & 2)
> -            {
> -                for (i = 0; i < msg->length; ++i)
> -                {
> -                    if (msg->data[i] == 0xF7)
> -                    {
> -                        FIXME("Ending System Exclusive\n");
> -                        src->state &= ~2;
> +
> +            bool sysexStart = (msg->data[0] == 0xF0);
> +

Wine uses C89, so we don't have 'bool'. You could change this to a
BOOL instead.

> +            if (sysexStart || src->state & 2) {
> +                if (sysexStart) {
> +                    TRACE("Receiving sysex message\n");
> +                    src->state |= 2;
> +                }
> +
> +                int pos = 0;
> +                int len = msg->length;
> +

C89 also doesn't let you declare new variables in the middle of a
block.  They must be declared at the start of a block.

> +                EnterCriticalSection(&midiInLock);
> +                currentTime = GetTickCount() - src->startTime;
> +
> +                while (len) {
> +                    LPMIDIHDR lpMidiHdr = src->lpQueueHdr;
> +
> +                    if (lpMidiHdr != NULL) {
> +                        int copylen = min(len, lpMidiHdr->dwBufferLength - lpMidiHdr->dwBytesRecorded);
> +                        memcpy(lpMidiHdr->lpData + lpMidiHdr->dwBytesRecorded, msg->data + pos, copylen);
> +                        lpMidiHdr->dwBytesRecorded += copylen;
> +                        len -= copylen;
> +                        pos += copylen;
> +
> +                        TRACE("Copied %d bytes of sysex message\n", copylen);
> +
> +                        if ((lpMidiHdr->dwBytesRecorded == lpMidiHdr->dwBufferLength) ||
> +				            (*(BYTE*)(lpMidiHdr->lpData + lpMidiHdr->dwBytesRecorded - 1) == 0xF7)) {
> +                            TRACE("Sysex message complete (or buffer limit reached), dispatching %d bytes\n", lpMidiHdr->dwBytesRecorded);

Tabs should be spaces, for consistency with the rest of your code.

> +                            src->lpQueueHdr = lpMidiHdr->lpNext;
> +                            lpMidiHdr->dwFlags &= ~MHDR_INQUEUE;
> +                            lpMidiHdr->dwFlags |= MHDR_DONE;
> +                            MIDI_NotifyClient(msg->devID, MIM_LONGDATA, (DWORD_PTR)lpMidiHdr, currentTime);
> +                            src->state &= ~2;
> +                        }
> +                    }
> +			        else {

Here, too.

> +                        FIXME("Sysex data received but no buffer to store it!\n");
> +                        break;
>                      }
>                  }
> -                goto done;
> +
> +                LeaveCriticalSection(&midiInLock);
> +                return NULL;
>              }
> +
>              EnterCriticalSection(&midiInLock);
>              currentTime = GetTickCount() - src->startTime;
>  
> @@ -889,7 +918,6 @@ static CFDataRef MIDIIn_MessageHandler(CFMessagePortRef local, SInt32 msgid, CFD
>              CFRunLoopStop(CFRunLoopGetCurrent());
>              break;
>      }
> -done:
>      return NULL;
>  }
>  
> -- 
> 1.9.3 (Apple Git-50)