Truncate MIDI SysEx messages after termination byte

Hi,

Johannes Kroll wrote:
> - for(i = 0; i < lpMidiOutHdr->dwBufferLength; i++)
> -
>    if((unsigned char)lpMidiOutHdr->lpData[i] == 0xF7 && i < lpMidiOutHdr->dwBufferLength-1)

What about
     for(i = 0; i+1 < lpMidiOutHdr->dwBufferLength, i++)
  without the redundant second if(&&) half?

I'm always happy when somebody with real MIDI HW speaks up, because
I'm not convinced that the Wine MIDI code does TRT about SysEx --
based on my readings, not experience, as I own no MIDI device...
In particular, do you have any experience with multi-part SysEx
messages (e.g. large data, such as uploads)?

Still, I'm not persuaded that your patch is at the right place.
I believe the midi* functions should be tiny wrappers around MODM_*
messages, same for the wave* functions.  Every time I see somebody
attempt to violate this 1:1 mapping, I'm suspicious.  Perhaps the
logic that you're adding belongs to the individual wine*.drv/midi.c?

Do you get to see this warning from winealsa.drv where it
specifically checks for a terminating F7 (but not intermediate F7)?
http://source.winehq.org/source/dlls/winealsa.drv/midi.c#L969
964     /* FIXME: MS doc is not 100% clear. Will lpData only contain system exclusive
965      * data, or can it also contain raw MIDI data, to be split up and sent to
966      * modShortData() ?
967      * If the latest is true, then the following WARNing will fire up
968      */
969     if (lpData[0] != 0xF0 || lpData[lpMidiHdr->dwBufferLength - 1] != 0xF7) {
970         WARN("Alleged system exclusive buffer is not correct\n\tPlease report with MIDI file\n");
971         lpNewData = HeapAlloc(GetProcessHeap(), 0, lpMidiHdr->dwBufferLength + 2);
972     }

Therefore, I'd be happy if you could invest some more time and check,
based on your real MIDI HW, and perhaps native w* machines,
whether MODM_LONGDATA and midiOutLongMsg are equivalent
or whether midiOut* does some additional processing.

Regards,
    Jörg Höhle

[Christian Costa]

> Still, I'm not persuaded that your patch is at the right place.
> I believe the midi* functions should be tiny wrappers around MODM_*
> messages, same for the wave* functions.  Every time I see somebody
> attempt to violate this 1:1 mapping, I'm suspicious.  Perhaps the
> logic that you're adding belongs to the individual wine*.drv/midi.c?

Maybe it worth compare visually between builtin and native by adding
sysex dump code to one driver if needed as 1st and last 3 bytes are
currently dump.
> Therefore, I'd be happy if you could invest some more time and check,
> based on your real MIDI HW, and perhaps native w* machines,
> whether MODM_LONGDATA and midiOutLongMsg are equivalent
> or whether midiOut* does some additional processing.

I don't know if Johannes uses alsa driver but I took a look at the
alsa code and this code simply does not do what it is supposed to.
I think that would be better to fix this code first as it might be the
cause of the bug.

984         if (lpData[0] != 0xF0) {
985             /* Send start of System Exclusive */
986             len_add = 1;
987             lpData[0] = 0xF0;
988             memcpy(lpNewData, lpData, lpMidiHdr->dwBufferLength);
989             WARN("Adding missing 0xF0 marker at the beginning of "
990                  "system exclusive byte stream\n");
991         }
992         if (lpData[lpMidiHdr->dwBufferLength-1] != 0xF7) {
993             /* Send end of System Exclusive */
994             memcpy(lpData + len_add, lpData, lpMidiHdr->dwBufferLength);
995             lpNewData[lpMidiHdr->dwBufferLength + len_add - 1] = 0xF0;
996             len_add++;
997             WARN("Adding missing 0xF7 marker at the end of "
998                  "system exclusive byte stream\n");

999 }

[Johannes Kroll]

On Mon, 14 Jan 2013 11:42:46 +0100
 wrote:
> Hi,
>
> Johannes Kroll wrote:
> > - for(i = 0; i < lpMidiOutHdr->dwBufferLength; i++)
> > -
> >    if((unsigned char)lpMidiOutHdr->lpData[i] == 0xF7 && i < lpMidiOutHdr->dwBufferLength-1)
>
> What about
>      for(i = 0; i+1 < lpMidiOutHdr->dwBufferLength, i++)
>   without the redundant second if(&&) half?

Sure. That patch was quickly written, I was just happy to have
something which 'made it work'.
> I'm always happy when somebody with real MIDI HW speaks up, because
> I'm not convinced that the Wine MIDI code does TRT about SysEx --
> based on my readings, not experience, as I own no MIDI device...
> In particular, do you have any experience with multi-part SysEx
> messages (e.g. large data, such as uploads)?

I don't think so, no. I have the nanokontrol/nanokey/nanopad combo,
but they don't take large data blocks such as samples if that's what
you mean. They're also USB-MIDI devices, which might explain some
specific quirks. I.e. at the place I tried to fix with my patch, a
traditional MIDI device might just stop at the 0xF7 and get the SysEx
message OK (but it would still be confused by the rest of the bytes in
the buffer, there is still something wrong).

I have a really old keyboard/synth here with traditional MIDI but I
think it also doesn't use multipart SysEx messages.
> Still, I'm not persuaded that your patch is at the right place.
> I believe the midi* functions should be tiny wrappers around MODM_*
> messages, same for the wave* functions.  Every time I see somebody
> attempt to violate this 1:1 mapping, I'm suspicious.  Perhaps the
> logic that you're adding belongs to the individual wine*.drv/midi.c?

I'm not familiar with Wine code so if you think the fix should go to
some other place you're probably right.

I will investigate some more later when I have some time. Thanks.
> Do you get to see this warning from winealsa.drv where it
> specifically checks for a terminating F7 (but not intermediate F7)?
> http://source.winehq.org/source/dlls/winealsa.drv/midi.c#L969
> 964     /* FIXME: MS doc is not 100% clear. Will lpData only contain system exclusive
> 965      * data, or can it also contain raw MIDI data, to be split up and sent to
> 966      * modShortData() ?
> 967      * If the latest is true, then the following WARNing will fire up
> 968      */
> 969     if (lpData[0] != 0xF0 || lpData[lpMidiHdr->dwBufferLength - 1] != 0xF7) {
> 970         WARN("Alleged system exclusive buffer is not correct\n\tPlease report with MIDI file\n");
> 971         lpNewData = HeapAlloc(GetProcessHeap(), 0, lpMidiHdr->dwBufferLength + 2);
> 972     }
>
> Therefore, I'd be happy if you could invest some more time and check,
> based on your real MIDI HW, and perhaps native w* machines,
> whether MODM_LONGDATA and midiOutLongMsg are equivalent
> or whether midiOut* does some additional processing.
>
> Regards,
>    Jörg Höhle

Hi,

Although I wouldn't mind inclusion of Christian's F0/F7 patch in upcoming Wine
(barring decent formatting), I beg and urge Johannes Kroll to nevertheless submit
a bug report about F0/F7 handling *and* provide a log of all SysEx messages sent
submitted by the app to Wine.

There's code to dump the data contents in another library that he could copy
to winealsa to produce the SysEx dump:
http://source.winehq.org/git/wine.git/blob/HEAD:/dlls/winmm/winmm.c#l1066

What becomes clear from below documents is that at least our mciseq .mid file parser
does not handle SysEx continuations aka. divided SysEx,
nor encapsulated SysEx aka. encapsulated events at all.
https://www.allegro.cc/forums/thread/607445
http://www.sonicspot.com/guide/midifiles.html

You could argue that .mid file parsing has no influence on winealsa.
However, as midiOutLongMsg and midiOutShortMsg are the only low-level API
calls available, the question is whether native is able to send such partial
SysEx to external HW devices.  Obviously, automatically padding with F0/F7
(which winealsa would like to do and wineoss does) prevents that.

Should one expect the native low-level drivers to split large SysEx and insert
pauses during transmission, or should the apps be able to split and send themselves?
The existence of SysEx continuations in MIDI files causes me to believe that the
same splitting should be possible on the output side, under control of the app.

It's also unclear how the backends would react to missing F0/F7 delimiters.
ALSA says: "the sysex data must contain the start byte 0xf0 and the end byte 0xf7."
http://www.alsa-project.org/alsa-doc/alsa-lib/group___seq_middle.html#ga1048...

Regards,
    Jörg Höhle

[Christian Costa]

2013/1/18  :
> Hi,
>
> Although I wouldn't mind inclusion of Christian's F0/F7 patch in upcoming Wine
> (barring decent formatting),

Well, it's not a formatting problem. It was intentional. The Wine
preferred style is 4 spaces indentation and no tab
but I didn't know that when I wrote the alsa midi driver. I took
wineoss one as a base without doing a cleanup.
I asked to Alexandre one day how to fix this kind of thing and he told
me to fix only the code I change as long as this fits
nicely with the code and it was for d3dxof which is 2 spaces
indentation. In this case it's a mix of tabs and space.
Replacing with space only looks the same and anyway last part of the
function is 4 spaces too. I admit I cleaned the whole function
whereas less than half of the code was changed. If something has
changed let me know.

Christian

[Christian Costa]

Le 18/01/2013 17:16, [e-mail] a écrit :
> Hi,
>
> Although I wouldn't mind inclusion of Christian's F0/F7 patch in upcoming Wine
> (barring decent formatting), I beg and urge Johannes Kroll to nevertheless submit
> a bug report about F0/F7 handling *and* provide a log of all SysEx messages sent
> submitted by the app to Wine.
>
> There's code to dump the data contents in another library that he could copy
> to winealsa to produce the SysEx dump:
> http://source.winehq.org/git/wine.git/blob/HEAD:/dlls/winmm/winmm.c#l1066
>
> What becomes clear from below documents is that at least our mciseq .mid file parser
> does not handle SysEx continuations aka. divided SysEx,
> nor encapsulated SysEx aka. encapsulated events at all.
> https://www.allegro.cc/forums/thread/607445
> http://www.sonicspot.com/guide/midifiles.html
>
> You could argue that .mid file parsing has no influence on winealsa.
> However, as midiOutLongMsg and midiOutShortMsg are the only low-level API
> calls available, the question is whether native is able to send such partial
> SysEx to external HW devices.  Obviously, automatically padding with F0/F7
> (which winealsa would like to do and wineoss does) prevents that.
>
> Should one expect the native low-level drivers to split large SysEx and insert
> pauses during transmission, or should the apps be able to split and send themselves?
> The existence of SysEx continuations in MIDI files causes me to believe that the
> same splitting should be possible on the output side, under control of the app.
>
> It's also unclear how the backends would react to missing F0/F7 delimiters.
> ALSA says: "the sysex data must contain the start byte 0xf0 and the end byte 0xf7."
> http://www.alsa-project.org/alsa-doc/alsa-lib/group___seq_middle.html#ga1048...
>
> Regards,
>    Jörg Höhle

One thing we can do it is to check what native winmm really does by 
writing a fake low level driver.
If we can register it dynamically like ICInstall from msvideo we can do 
all the checks we want in the tests.

Bye
Christian

[Christian Costa]

Le 18/01/2013 17:16, [e-mail] a écrit :
> Hi,
>
> Although I wouldn't mind inclusion of Christian's F0/F7 patch in upcoming Wine
> (barring decent formatting), I beg and urge Johannes Kroll to nevertheless submit
> a bug report about F0/F7 handling *and* provide a log of all SysEx messages sent
> submitted by the app to Wine.
>
> There's code to dump the data contents in another library that he could copy
> to winealsa to produce the SysEx dump:
> http://source.winehq.org/git/wine.git/blob/HEAD:/dlls/winmm/winmm.c#l1066
>
> What becomes clear from below documents is that at least our mciseq .mid file parser
> does not handle SysEx continuations aka. divided SysEx,
> nor encapsulated SysEx aka. encapsulated events at all.
> https://www.allegro.cc/forums/thread/607445
> http://www.sonicspot.com/guide/midifiles.html
>
> You could argue that .mid file parsing has no influence on winealsa.
> However, as midiOutLongMsg and midiOutShortMsg are the only low-level API
> calls available, the question is whether native is able to send such partial
> SysEx to external HW devices.  Obviously, automatically padding with F0/F7
> (which winealsa would like to do and wineoss does) prevents that.
>
> Should one expect the native low-level drivers to split large SysEx and insert
> pauses during transmission, or should the apps be able to split and send themselves?
> The existence of SysEx continuations in MIDI files causes me to believe that the
> same splitting should be possible on the output side, under control of the app.
>
> It's also unclear how the backends would react to missing F0/F7 delimiters.
> ALSA says: "the sysex data must contain the start byte 0xf0 and the end byte 0xf7."
> http://www.alsa-project.org/alsa-doc/alsa-lib/group___seq_middle.html#ga1048...
>
> Regards,
>    Jörg Höhle

One thing we can do it is to check what native winmm really does by 
writing a fake low level driver.
If we can register it dynamically like ICInstall from msvideo we can do 
all the checks we want in the tests.
Maybe winmm does more things that we think.

Bye
Christian