Re: Truncate MIDI SysEx messages after termination byte
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
Re: Truncate MIDI SysEx messages after termination byte
2013/1/15 : > Christian costa wrote: >>> But I don't know whether there's some harmless general SysEx that we >>> can send out. Maybe some MIDI guru could suggest some? >>What do you by harmless ? For what ? A midi HW device connected to the midi >>port ? > > A harmless MIDI message, perhaps there exists a NOP SysEx? > E.g. a reset command is *not* harmless. > > Here are a few SysEx I gathered, looking for MIDI SysEx documentation > Proteus SoundEngine System Reset SysEx F0 18 04 00 23 F7 > without knowing what they do: > F0 56 64 4F 6C F7 > F0 7D 12 34 10 02 11 00 F7 > SysEx are quite device specific and I don't kwnon about nop sysex. Anyway I can take one of these. >>we cannot verify the output. > Sure, however increased code coverage *has* benefits. E.g. sometimes I use > Valgrind. > Ok. Makes sense.
Truncate MIDI SysEx messages after termination byte
Christian costa wrote: >> But I don't know whether there's some harmless general SysEx that we >> can send out. Maybe some MIDI guru could suggest some? >What do you by harmless ? For what ? A midi HW device connected to the midi >port ? A harmless MIDI message, perhaps there exists a NOP SysEx? E.g. a reset command is *not* harmless. Here are a few SysEx I gathered, looking for MIDI SysEx documentation Proteus SoundEngine System Reset SysEx F0 18 04 00 23 F7 without knowing what they do: F0 56 64 4F 6C F7 F0 7D 12 34 10 02 11 00 F7 >we cannot verify the output. Sure, however increased code coverage *has* benefits. E.g. sometimes I use Valgrind. Regards, Jörg
Re: Truncate MIDI SysEx messages after termination byte
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
Re: Truncate MIDI SysEx messages after termination byte
Hi, 2013/1/15 : > Hi, > > Christian costa wrote: >>+lpNewData[lpMidiHdr->dwBufferLength + len_add] = 0xF7; > Here you're using spaces only, whereas the surrounding code uses TAB8. It was not intended for submission. If I submit a patch, I will removed all these tabs. > >>I tested it with the unit test attached. > Instead of some isolated unit test, it would be preferable to > add to winmm/tests/midi.c such that midiOutLongMsg gets covered > by the tests. But I don't know whether there's some harmless > general SysEx that we can send out. Maybe some MIDI guru could > suggest some? > The unit test was just to check byte insertion code. I attached for info only. I could add a test but this will be only for debug purpose as we cannot verify the output. What do you by harmless ? For what ? A midi HW device connected to the midi port ? > > The fix looks good, however while we are busy fixing lpNewData handling, > I'd like to see the same patch fix its broken memory handling: > > 1. In the MOD_FMSYNTH branch, no > HeapFree(GetProcessHeap(), 0, lpNewData); > Here I'm surprised that the static analysers did not spot it. > > 2. Wrong pointer being freed -- is that an indirect proof by absence of > crash that this missing F0/F7 situation was never seen in practice? > 1007 if (lpNewData) > 1008 HeapFree(GetProcessHeap(), 0, lpData); > Also, Wine style is to suppress the if() > > 3. Not checking for success > 971 lpNewData = HeapAlloc(GetProcessHeap(), 0, > lpMidiHdr->dwBufferLength + 2); > Either exit or skip adding F0/F7 > I saw the memory leak. That needs to be fixed as well. And indeed, this code is definitely broken and I doubt it has been ever exercised unless it has been coded first and them break afterwards. Christian
Truncate MIDI SysEx messages after termination byte
Hi, Christian costa wrote: >+lpNewData[lpMidiHdr->dwBufferLength + len_add] = 0xF7; Here you're using spaces only, whereas the surrounding code uses TAB8. >I tested it with the unit test attached. Instead of some isolated unit test, it would be preferable to add to winmm/tests/midi.c such that midiOutLongMsg gets covered by the tests. But I don't know whether there's some harmless general SysEx that we can send out. Maybe some MIDI guru could suggest some? The fix looks good, however while we are busy fixing lpNewData handling, I'd like to see the same patch fix its broken memory handling: 1. In the MOD_FMSYNTH branch, no HeapFree(GetProcessHeap(), 0, lpNewData); Here I'm surprised that the static analysers did not spot it. 2. Wrong pointer being freed -- is that an indirect proof by absence of crash that this missing F0/F7 situation was never seen in practice? 1007 if (lpNewData) 1008 HeapFree(GetProcessHeap(), 0, lpData); Also, Wine style is to suppress the if() 3. Not checking for success 971 lpNewData = HeapAlloc(GetProcessHeap(), 0, lpMidiHdr->dwBufferLength + 2); Either exit or skip adding F0/F7 Regards, Jörg Höhle
Re: Truncate MIDI SysEx messages after termination byte
Le 14/01/2013 19:04, Christian Costa a écrit :
2013/1/14 Christian Costa :
2013/1/14 :
Hi,
Christian Costa wrote:
I took a look at the
alsa code and this code simply does not do what it is supposed to.
I also looked at it today and noted those bogus lines you quote. Needs
a patch (+ fix memory leak).
I will take a look at it and send a patch unless someone plan to do it.
I will not be able to test with my 2 HW midi devices right now so I need
someone to test it. That would be good to enter a bug report for that.
Johannes, can you do that?
However, Johannes' change is presumably different, as he wants to
scan the buffer contents for a F7 terminator and ignore subsequent
bytes. If I were to decide, I'd like to see more supporting evidence.
In the case the first by is F0 and the last one is F7, the code passes
I meant "last one if not F7".
Here are a patch to fix modLongData.
I tested it with the unit test attached.
Please give it a try.
diff --git a/dlls/winealsa.drv/midi.c b/dlls/winealsa.drv/midi.c
index 7c496c5..3f895e9 100644
--- a/dlls/winealsa.drv/midi.c
+++ b/dlls/winealsa.drv/midi.c
@@ -984,15 +984,16 @@ static DWORD modLongData(WORD wDevID, LPMIDIHDR lpMidiHdr, DWORD dwSize)
if (lpData[0] != 0xF0) {
/* Send start of System Exclusive */
len_add = 1;
- lpData[0] = 0xF0;
- memcpy(lpNewData, lpData, lpMidiHdr->dwBufferLength);
+ lpNewData[0] = 0xF0;
+ memcpy(lpNewData + 1, lpData, lpMidiHdr->dwBufferLength);
WARN("Adding missing 0xF0 marker at the beginning of "
"system exclusive byte stream\n");
}
if (lpData[lpMidiHdr->dwBufferLength-1] != 0xF7) {
/* Send end of System Exclusive */
- memcpy(lpData + len_add, lpData, lpMidiHdr->dwBufferLength);
-lpNewData[lpMidiHdr->dwBufferLength + len_add - 1] = 0xF0;
+ if (!len_add)
+ memcpy(lpNewData, lpData, lpMidiHdr->dwBufferLength);
+lpNewData[lpMidiHdr->dwBufferLength + len_add] = 0xF7;
len_add++;
WARN("Adding missing 0xF7 marker at the end of "
"system exclusive byte stream\n");
#include
#include
#include
unsigned char data[] = {0xF0, 1, 2, 3, 4, 0xF6};
int main(void)
{
int len_add = 0;
unsigned char *lpData = data, *lpNewData = NULL;
int dwBufferLength = sizeof(data);
int i = 0;
for (i = 0; i < dwBufferLength; i++)
printf("%#x ", data[i]);
printf("\n");
if (lpData[0] != 0xF0 || lpData[dwBufferLength - 1] != 0xF7) {
printf("Alleged system exclusive buffer is not correct\n\tPlease report with MIDI file\n");
lpNewData = malloc(dwBufferLength + 2);
}
if (lpData[0] != 0xF0) {
/* Send start of System Exclusive */
len_add = 1;
lpNewData[0] = 0xF0;
memcpy(lpNewData + 1, lpData, dwBufferLength);
printf("Adding missing 0xF0 marker at the beginning of "
"system exclusive byte stream\n");
}
if (lpData[dwBufferLength-1] != 0xF7) {
/* Send end of System Exclusive */
if (!len_add)
memcpy(lpNewData, lpData, dwBufferLength);
lpNewData[dwBufferLength + len_add] = 0xF7;
len_add++;
printf("Adding missing 0xF7 marker at the end of "
"system exclusive byte stream\n");
}
for (i = 0; i < dwBufferLength + len_add; i++)
printf("%#x ", lpNewData ? lpNewData[i] : lpData[i]);
printf("\n");
}
Re: Truncate MIDI SysEx messages after termination byte
2013/1/14 Christian Costa : > 2013/1/14 : >> Hi, >> >> Christian Costa wrote: >>>I took a look at the >>>alsa code and this code simply does not do what it is supposed to. >> I also looked at it today and noted those bogus lines you quote. Needs >> a patch (+ fix memory leak). > > I will take a look at it and send a patch unless someone plan to do it. > I will not be able to test with my 2 HW midi devices right now so I need > someone to test it. That would be good to enter a bug report for that. > Johannes, can you do that? > >> >> However, Johannes' change is presumably different, as he wants to >> scan the buffer contents for a F7 terminator and ignore subsequent >> bytes. If I were to decide, I'd like to see more supporting evidence. > > In the case the first by is F0 and the last one is F7, the code passes I meant "last one if not F7".
Re: Truncate MIDI SysEx messages after termination byte
2013/1/14 : > Hi, > > Christian Costa wrote: >>I took a look at the >>alsa code and this code simply does not do what it is supposed to. > I also looked at it today and noted those bogus lines you quote. Needs > a patch (+ fix memory leak). I will take a look at it and send a patch unless someone plan to do it. I will not be able to test with my 2 HW midi devices right now so I need someone to test it. That would be good to enter a bug report for that. Johannes, can you do that? > > However, Johannes' change is presumably different, as he wants to > scan the buffer contents for a F7 terminator and ignore subsequent > bytes. If I were to decide, I'd like to see more supporting evidence. In the case the first by is F0 and the last one is F7, the code passes a uninitialized buffer to the alsa function with a wrong terminating byte F0 inserted which is even worst because it opens a new sys ex sequence without end. The Johanness patch prevents this case to happend by making last byte to be F7. Bye Christian
Truncate MIDI SysEx messages after termination byte
Hi, Christian Costa wrote: >I took a look at the >alsa code and this code simply does not do what it is supposed to. I also looked at it today and noted those bogus lines you quote. Needs a patch (+ fix memory leak). However, Johannes' change is presumably different, as he wants to scan the buffer contents for a F7 terminator and ignore subsequent bytes. If I were to decide, I'd like to see more supporting evidence. I'd appreciate if Johannes would open a bug report and attach some WINEDEBUG=+midi,+mmdevapi,+alsa,+winmm,+driver,+tid,+timestamp logs to bugzilla, much like bug #26928. http://bugs.winehq.org/show_bug.cgi?id=26928 Having some logs with *complete* SysEx contents helps. There's also a "closed as abandoned" bug about microKORG http://bugs.winehq.org/show_bug.cgi?id=17608 which might be of interest to Johannes. Regards, Jörg Höhle
Re: Truncate MIDI SysEx messages after termination byte
> 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 }
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
Re: Truncate MIDI SysEx messages after termination byte
2013/1/10 Johannes Kroll
> On Thu, 10 Jan 2013 12:04:02 +0100
> Christian Costa wrote:
>
> > In your code you stop checking F7 just before the last byte (until
> > position lpMidiOutHdr->dwBufferLength-2);
> > You can check the last byte as well. This is also valid and simplify the
> > condition.
> > The missing F7 byte case is handled by the for loop.
>
> Ah, you mean that check. Yes, it's not strictly necessary, it just
> decides whether it has to save & restore dwBufferSize or not.
>
>
> > Regarding the single MMDRV_Message call, I was thinking about something
> > like below.
> > This also enable printing a WARN when F7 is missing which can be useful
> for
> > debug.
> >
> > DWORD oldBufferLength = lpMidiOutHdr->dwBufferLength;
> > DWORD ret;
> >
> > for(i = 0; i < lpMidiOutHdr->dwBufferLength; i++)
> >
> >/* SysEx messages are terminated by a 0xF7 byte. If the buffer
> > contains additional
> > bytes, send only the bytes up to the termination byte. */
> > if((unsigned char)lpMidiOutHdr->lpData[i] == 0xF7)
> > {
> > lpMidiOutHdr->dwBufferLength = i+1;
> >
> > break;
> >
> > }
> >
> >
> > if (i == lpMidiOutHdr->dwBufferLength)
> >
> > WARN("SysEx termination byte 0xF7 missing\n")
> >
> >
> >
> > ret = MMDRV_Message(wmld, MODM_LONGDATA, (DWORD_PTR)lpMidiOutHdr,
> > uSize);
> >
> > /* restore the midi header to its original state. */
> > lpMidiOutHdr->dwBufferLength = oldBufferLength;
> >
> >
> > return ret;
>
> I see nothing wrong with that apart from the broken formatting (please
> don't use HTML mail). Did you test it?
>
>
It was not intended to be used as is. It's just to show what I had in mind.
> I don't care which version is used, I would just be very happy if
> *something* is included which makes my MIDI device work with Wine.
>
There is no reason your patch will not get in. Generally, it's just patches
need
to have the quality required to be committed. Sometime it can takes several
iterations.
>
> How does this generally work here, will there be feedback from a
> maintainer on whether the patch will be included, or is it usually just
> silently included?
> Alexandre Julliard (or whoever decides this): Could you comment please?
>
>
>
For a patch which is ok, it will be committed silently by Alexandre. You
may already
know the patches status page at http://source.winehq.org/patches.
Usually maintainers or skilled people of a specific area look at patches and
give some feedback if something seems wrong or not clear.
> And Christian, please, configure your mail client. The list mails I get
> from you are directed to me, not the list, and they are missing the
> List-Id field.
>
>
> When I use gmail web client, I don't have good results sometimes. Maybe
there is a configuration for that.
I'm full open to any hint.
Regarding emails addresses, I write to someone with wine-devel in cc but
I'm ok to use
only wine-devel with you.
Re: Truncate MIDI SysEx messages after termination byte
On Thu, 10 Jan 2013 12:04:02 +0100
Christian Costa wrote:
> In your code you stop checking F7 just before the last byte (until
> position lpMidiOutHdr->dwBufferLength-2);
> You can check the last byte as well. This is also valid and simplify the
> condition.
> The missing F7 byte case is handled by the for loop.
Ah, you mean that check. Yes, it's not strictly necessary, it just
decides whether it has to save & restore dwBufferSize or not.
> Regarding the single MMDRV_Message call, I was thinking about something
> like below.
> This also enable printing a WARN when F7 is missing which can be useful for
> debug.
>
> DWORD oldBufferLength = lpMidiOutHdr->dwBufferLength;
> DWORD ret;
>
> for(i = 0; i < lpMidiOutHdr->dwBufferLength; i++)
>
>/* SysEx messages are terminated by a 0xF7 byte. If the buffer
> contains additional
> bytes, send only the bytes up to the termination byte. */
> if((unsigned char)lpMidiOutHdr->lpData[i] == 0xF7)
> {
> lpMidiOutHdr->dwBufferLength = i+1;
>
> break;
>
> }
>
>
> if (i == lpMidiOutHdr->dwBufferLength)
>
> WARN("SysEx termination byte 0xF7 missing\n")
>
>
>
> ret = MMDRV_Message(wmld, MODM_LONGDATA, (DWORD_PTR)lpMidiOutHdr,
> uSize);
>
> /* restore the midi header to its original state. */
> lpMidiOutHdr->dwBufferLength = oldBufferLength;
>
>
> return ret;
I see nothing wrong with that apart from the broken formatting (please
don't use HTML mail). Did you test it?
I don't care which version is used, I would just be very happy if
*something* is included which makes my MIDI device work with Wine.
How does this generally work here, will there be feedback from a
maintainer on whether the patch will be included, or is it usually just
silently included?
Alexandre Julliard (or whoever decides this): Could you comment please?
And Christian, please, configure your mail client. The list mails I get
from you are directed to me, not the list, and they are missing the
List-Id field.
Re: Truncate MIDI SysEx messages after termination byte
2013/1/10 Johannes Kroll
> On Thu, 10 Jan 2013 01:05:28 +0100
> Johannes Kroll wrote:
>
> > On Thu, 10 Jan 2013 00:38:06 +0100
> > Christian Costa wrote:
> >
> >
> > > After a better look the changes seem correct.
> > > The condition i < lpMidioutHdr->dwBufferLength-1 is not necessary
> tough.
> >
> > Without the buffer length check, there would be a buffer overflow when
> > an application tries to send a SysEx without an F7 byte.
>
> I meant a read past the end of the buffer of course, not an overflow.
>
>
>
In your code you stop checking F7 just before the last byte (until
position lpMidiOutHdr->dwBufferLength-2);
You can check the last byte as well. This is also valid and simplify the
condition.
The missing F7 byte case is handled by the for loop.
Regarding the single MMDRV_Message call, I was thinking about something
like below.
This also enable printing a WARN when F7 is missing which can be useful for
debug.
DWORD oldBufferLength = lpMidiOutHdr->dwBufferLength;
DWORD ret;
for(i = 0; i < lpMidiOutHdr->dwBufferLength; i++)
/* SysEx messages are terminated by a 0xF7 byte. If the buffer
contains additional
bytes, send only the bytes up to the termination byte. */
if((unsigned char)lpMidiOutHdr->lpData[i] == 0xF7)
{
lpMidiOutHdr->dwBufferLength = i+1;
break;
}
if (i == lpMidiOutHdr->dwBufferLength)
WARN("SysEx termination byte 0xF7 missing\n")
ret = MMDRV_Message(wmld, MODM_LONGDATA, (DWORD_PTR)lpMidiOutHdr,
uSize);
/* restore the midi header to its original state. */
lpMidiOutHdr->dwBufferLength = oldBufferLength;
return ret;
Re: Truncate MIDI SysEx messages after termination byte
On Thu, 10 Jan 2013 01:05:28 +0100 Johannes Kroll wrote: > On Thu, 10 Jan 2013 00:38:06 +0100 > Christian Costa wrote: > > > > After a better look the changes seem correct. > > The condition i < lpMidioutHdr->dwBufferLength-1 is not necessary tough. > > Without the buffer length check, there would be a buffer overflow when > an application tries to send a SysEx without an F7 byte. I meant a read past the end of the buffer of course, not an overflow.
Re: Truncate MIDI SysEx messages after termination byte
On Thu, 10 Jan 2013 00:38:06 +0100 Christian Costa wrote: > After a better look the changes seem correct. > The condition i < lpMidioutHdr->dwBufferLength-1 is not necessary tough. Without the buffer length check, there would be a buffer overflow when an application tries to send a SysEx without an F7 byte.
Re: Truncate MIDI SysEx messages after termination byte
Le 09/01/2013 19:32, Johannes Kroll a écrit : On Wed, 9 Jan 2013 19:04:03 +0100 Christian Costa wrote: I would say consistency would be 2 spaces or none. You are, of course, entitled to your opinion. If my opinion does not matter as well as code style practices we can found in wine that would be better to stick to file code style. Off course you're free to submit what you want. Using only 1 space for assignment does not seem very common in wine AFAIK. In general it's better to follow the file style or the function to avoid mixing styles except if you work heavyly on these files. If anybody sees non-style-related issues with the patch, please let me know. dwBytesRecorded is supposed to tell how many bytes are valid. What's wrong with it ? Is it? It was always zero in midiOutLongMsg when I checked. The program works on Windows though. This means that either dwBytesRecorded is not supposed to be valid in that place, possibly only when data is read from some MIDI device. This would seem to be consistent with the "-Recorded" in the variable name. Or it is supposed to be valid, which would mean that there is a bug somewhere else in Wine. SysEx messages where the real message length equals dwBufferSize do get through properly even without the patch, which means that the code in MMDRV_Message and onwards does not care about dwBytesRecorded. It uses dwBufferSize to check how many bytes it should send. This applies only for recording which is not the case here. After a better look the changes seem correct. The condition i < lpMidioutHdr->dwBufferLength-1 is not necessary tough. And a single call to MMDRV_Message would be better. Please keep any replies on the list(s). Good mail client which sucks. This sentence makes no sense. Yours too. wine-patches is only for patch submission. Use -devel then. Just don't send me copies. ...
Re: Truncate MIDI SysEx messages after termination byte
On Wed, 9 Jan 2013 19:04:03 +0100 Christian Costa wrote: > I would say consistency would be 2 spaces or none. You are, of course, entitled to your opinion. > Using only 1 space for assignment does not seem very common in wine AFAIK. > In general it's better to follow the file style or the function to avoid > mixing styles > except if you work heavyly on these files. > > > > If anybody sees non-style-related issues with the patch, please let me > > know. > > > > dwBytesRecorded is supposed to tell how many bytes are valid. What's wrong > with it ? Is it? It was always zero in midiOutLongMsg when I checked. The program works on Windows though. This means that either dwBytesRecorded is not supposed to be valid in that place, possibly only when data is read from some MIDI device. This would seem to be consistent with the "-Recorded" in the variable name. Or it is supposed to be valid, which would mean that there is a bug somewhere else in Wine. SysEx messages where the real message length equals dwBufferSize do get through properly even without the patch, which means that the code in MMDRV_Message and onwards does not care about dwBytesRecorded. It uses dwBufferSize to check how many bytes it should send. > > Please keep any replies on the list(s). > > > > > Good mail client which sucks. This sentence makes no sense. > wine-patches is only for patch submission. Use -devel then. Just don't send me copies.
Re: Truncate MIDI SysEx messages after termination byte
2013/1/9 Johannes Kroll > On Wed, 9 Jan 2013 16:10:18 +0100 > Christian Costa wrote: > > > 2013/1/9 Johannes Kroll > > > > > On Wed, 09 Jan 2013 10:19:54 +0100 > > > Christian Costa wrote: > > > > > > > Le 09/01/2013 04:26, Johannes Kroll a écrit : > > > > > Hi, > > > > > > > > > > On Wed, 09 Jan 2013 03:55:25 +0100 > > > > > Christian Costa wrote: > > > > > > > > > >> Hi, > > > > >> > > > > >> Please be consistent when using space. 1 space before and after > > > > >> operators (<, ==, =, ...). > > > > > spaces inserted. HTH. > > > > > > > > > > J. > > > > That's better. You forgot some occurences in the for loop though. > > > > > > Is it OK if I leave the rest of the formatting to you? Any style is > > > fine with me, whatever makes you happy and hopefully gets the patch > > > included. > > > > > > Formatting only patch are not allowed. Clean patch and consistency in > code > > style help your patch get in. > > Alexandre Julliard is the only judge not me. If he is ok with your patch > > that's fine. > > Anyway having the for loop consistent with rest should not be that hard. > > > > for(i= 0; idwBufferLength; i++) > > The original patch was consistent with my coding style. To make > assignments stick out visually, I use no space to the left of the > assignment operator, one space to the right. On comparison operators, I > use space either on both sides, or none. You will see this in all code > I write. > > I attached another version of the patch. I added 3 more spaces and one > line break in a long comment line. You or Alexandre can choose the > version they like best. If you still see style issues, please consider > changing the style to your liking. If that's not possible, the patch > will have to be rejected. > > I would say consistency would be 2 spaces or none. Using only 1 space for assignment does not seem very common in wine AFAIK. In general it's better to follow the file style or the function to avoid mixing styles except if you work heavyly on these files. > If anybody sees non-style-related issues with the patch, please let me > know. > dwBytesRecorded is supposed to tell how many bytes are valid. What's wrong with it ? > > Please keep any replies on the list(s). > > Good mail client which sucks. wine-patches is only for patch submission. > Have a nice day. > Johannes > > > > > >
Re: Truncate MIDI SysEx messages after termination byte
On Wed, 9 Jan 2013 16:10:18 +0100
Christian Costa wrote:
> 2013/1/9 Johannes Kroll
>
> > On Wed, 09 Jan 2013 10:19:54 +0100
> > Christian Costa wrote:
> >
> > > Le 09/01/2013 04:26, Johannes Kroll a écrit :
> > > > Hi,
> > > >
> > > > On Wed, 09 Jan 2013 03:55:25 +0100
> > > > Christian Costa wrote:
> > > >
> > > >> Hi,
> > > >>
> > > >> Please be consistent when using space. 1 space before and after
> > > >> operators (<, ==, =, ...).
> > > > spaces inserted. HTH.
> > > >
> > > > J.
> > > That's better. You forgot some occurences in the for loop though.
> >
> > Is it OK if I leave the rest of the formatting to you? Any style is
> > fine with me, whatever makes you happy and hopefully gets the patch
> > included.
> >
> > Formatting only patch are not allowed. Clean patch and consistency in code
> style help your patch get in.
> Alexandre Julliard is the only judge not me. If he is ok with your patch
> that's fine.
> Anyway having the for loop consistent with rest should not be that hard.
>
> for(i= 0; idwBufferLength; i++)
The original patch was consistent with my coding style. To make
assignments stick out visually, I use no space to the left of the
assignment operator, one space to the right. On comparison operators, I
use space either on both sides, or none. You will see this in all code
I write.
I attached another version of the patch. I added 3 more spaces and one
line break in a long comment line. You or Alexandre can choose the
version they like best. If you still see style issues, please consider
changing the style to your liking. If that's not possible, the patch
will have to be rejected.
If anybody sees non-style-related issues with the patch, please let me
know.
Please keep any replies on the list(s).
Have a nice day.
Johannes
>From b8b97bb56dd6460b2b422061cc565dafe9d94d46 Mon Sep 17 00:00:00 2001
From: Johannes Kroll
Date: Wed, 9 Jan 2013 02:40:34 +0100
Subject: terminate sysex messages
---
dlls/winmm/winmm.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/dlls/winmm/winmm.c b/dlls/winmm/winmm.c
index aecb9cf..f4fecb1 100644
--- a/dlls/winmm/winmm.c
+++ b/dlls/winmm/winmm.c
@@ -531,11 +531,28 @@ UINT WINAPI midiOutLongMsg(HMIDIOUT hMidiOut,
MIDIHDR* lpMidiOutHdr, UINT uSize)
{
LPWINE_MLD wmld;
+int i;
TRACE("(%p, %p, %d)\n", hMidiOut, lpMidiOutHdr, uSize);
if ((wmld = MMDRV_Get(hMidiOut, MMDRV_MIDIOUT, FALSE)) == NULL)
return MMSYSERR_INVALHANDLE;
+
+for(i = 0; i < lpMidiOutHdr->dwBufferLength; i++)
+{
+/* SysEx messages are terminated by a 0xF7 byte. If the buffer
contains additional
+ bytes, send only the bytes up to the termination byte. */
+if((unsigned char)lpMidiOutHdr->lpData[i] == 0xF7 && i <
lpMidiOutHdr->dwBufferLength-1)
+{
+DWORD oldBufferLength = lpMidiOutHdr->dwBufferLength;
+DWORD ret;
+lpMidiOutHdr->dwBufferLength = i+1;
+ret = MMDRV_Message(wmld, MODM_LONGDATA, (DWORD_PTR)lpMidiOutHdr,
uSize);
+/* restore the midi header to its original state. */
+lpMidiOutHdr->dwBufferLength = oldBufferLength;
+return ret;
+}
+}
return MMDRV_Message(wmld, MODM_LONGDATA, (DWORD_PTR)lpMidiOutHdr, uSize);
}
--
1.7.9.5
Re: Truncate MIDI SysEx messages after termination byte
On Wed, 09 Jan 2013 10:19:54 +0100 Christian Costa wrote: > Le 09/01/2013 04:26, Johannes Kroll a écrit : > > Hi, > > > > On Wed, 09 Jan 2013 03:55:25 +0100 > > Christian Costa wrote: > > > >> Hi, > >> > >> Please be consistent when using space. 1 space before and after > >> operators (<, ==, =, ...). > > spaces inserted. HTH. > > > > J. > That's better. You forgot some occurences in the for loop though. Is it OK if I leave the rest of the formatting to you? Any style is fine with me, whatever makes you happy and hopefully gets the patch included.
Re: Truncate MIDI SysEx messages after termination byte
Le 09/01/2013 04:26, Johannes Kroll a écrit : Hi, On Wed, 09 Jan 2013 03:55:25 +0100 Christian Costa wrote: Hi, Please be consistent when using space. 1 space before and after operators (<, ==, =, ...). spaces inserted. HTH. J. That's better. You forgot some occurences in the for loop though. Christian
Re: Truncate MIDI SysEx messages after termination byte
Hi,
On Wed, 09 Jan 2013 03:55:25 +0100
Christian Costa wrote:
> Hi,
>
> Please be consistent when using space. 1 space before and after
> operators (<, ==, =, ...).
spaces inserted. HTH.
J.>From edd15d7d71e7309937c27dd9e6eab56bf42cc589 Mon Sep 17 00:00:00 2001
From: Johannes Kroll
Date: Wed, 9 Jan 2013 02:40:34 +0100
Subject: terminate sysex messages
---
dlls/winmm/winmm.c | 16
1 file changed, 16 insertions(+)
diff --git a/dlls/winmm/winmm.c b/dlls/winmm/winmm.c
index aecb9cf..1f8ea10 100644
--- a/dlls/winmm/winmm.c
+++ b/dlls/winmm/winmm.c
@@ -531,11 +531,27 @@ UINT WINAPI midiOutLongMsg(HMIDIOUT hMidiOut,
MIDIHDR* lpMidiOutHdr, UINT uSize)
{
LPWINE_MLD wmld;
+int i;
TRACE("(%p, %p, %d)\n", hMidiOut, lpMidiOutHdr, uSize);
if ((wmld = MMDRV_Get(hMidiOut, MMDRV_MIDIOUT, FALSE)) == NULL)
return MMSYSERR_INVALHANDLE;
+
+for(i= 0; idwBufferLength; i++)
+{
+/* SysEx messages are terminated by a 0xF7 byte. If the buffer
contains additional bytes, send only the bytes up to the termination byte. */
+if((unsigned char)lpMidiOutHdr->lpData[i] == 0xF7 && i <
lpMidiOutHdr->dwBufferLength-1)
+{
+DWORD oldBufferLength = lpMidiOutHdr->dwBufferLength;
+DWORD ret;
+lpMidiOutHdr->dwBufferLength = i+1;
+ret = MMDRV_Message(wmld, MODM_LONGDATA, (DWORD_PTR)lpMidiOutHdr,
uSize);
+/* restore the midi header to its original state. */
+lpMidiOutHdr->dwBufferLength = oldBufferLength;
+return ret;
+}
+}
return MMDRV_Message(wmld, MODM_LONGDATA, (DWORD_PTR)lpMidiOutHdr, uSize);
}
--
1.7.9.5
Re: Truncate MIDI SysEx messages after termination byte
Le 09/01/2013 03:12, Johannes Kroll a écrit : While using the KORG Kontrol Editor software [1] under Wine to configure my nanoPAD2 MIDI controller, I noticed that uploading parameters to the hardware doesn't work. The program displays a timeout message. The program relies on the MIDI driver to remove data after the SysEx termination byte [2], but the Wine's winmm implementation doesn't do this. The complete MIDIHDR buffer is sent, including excess bytes in the case when the buffer is larger than the MIDI message. The attached patch fixes the problem by truncating SysEx messages after the termination byte, ignoring any excess bytes. I tested the patch with KORG Kontrol Editor under Ubuntu 12.04 64-bit (32-bit wine). [1] http://www.korg.co.uk/support/downloads/nano2_dl.php [2] http://home.roadrunner.com/~jgglatt/tech/midispec/sysex.htm Hi, Please be consistent when using space. 1 space before and after operators (<, ==, =, ...). Christian
