[PATCH] Revert "winealsa.drv: Send MIDI events to port subscribers."

[Masanori Kakura]

This reverts commit 3d57cc2863f2f9a5ace40d29317b3ff4357fd119.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52220
Signed-off-by: Masanori Kakura 
---
 dlls/winealsa.drv/midi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/dlls/winealsa.drv/midi.c b/dlls/winealsa.drv/midi.c
index 073c0a6e3f3..74512ad09d2 100644
--- a/dlls/winealsa.drv/midi.c
+++ b/dlls/winealsa.drv/midi.c
@@ -869,7 +869,7 @@ static DWORD modData(WORD wDevID, DWORD dwParam)
             snd_seq_ev_clear(&event);
             snd_seq_ev_set_direct(&event);
             snd_seq_ev_set_source(&event, port_out);
-            snd_seq_ev_set_subs(&event);
+            snd_seq_ev_set_dest(&event, MidiOutDev[wDevID].addr.client, MidiOutDev[wDevID].addr.port);
        
        switch (evt & 0xF0) {
        case MIDI_CMD_NOTE_OFF:
@@ -1038,7 +1038,8 @@ static DWORD modLongData(WORD wDevID, LPMIDIHDR lpMidiHdr, DWORD dwSize)
    snd_seq_ev_clear(&event);
    snd_seq_ev_set_direct(&event);
    snd_seq_ev_set_source(&event, port_out);
-	snd_seq_ev_set_subs(&event);
+	snd_seq_ev_set_dest(&event, MidiOutDev[wDevID].addr.client, MidiOutDev[wDevID].addr.port);
+	TRACE("destination %d:%d\n", MidiOutDev[wDevID].addr.client, MidiOutDev[wDevID].addr.port);
    snd_seq_ev_set_sysex(&event, lpMidiHdr->dwBufferLength + len_add, lpNewData ? lpNewData : lpData);
         EnterCriticalSection(&midiSeqLock);
    snd_seq_event_output_direct(midiSeq, &event);

[Andrew Eikum]

On Thu, Dec 16, 2021 at 11:21:11PM +0900, Masanori Kakura wrote:
> This reverts commit 3d57cc2863f2f9a5ace40d29317b3ff4357fd119.
>
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52220
> Signed-off-by: Masanori Kakura

Thanks for reporting and looking into this, Kakurasan.

Jacek, this patch was sent by you. Can you take a look at this bug and
give us your thoughts?

Thank you,
Andrew
> dlls/winealsa.drv/midi.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/winealsa.drv/midi.c b/dlls/winealsa.drv/midi.c
> index 073c0a6e3f3..74512ad09d2 100644
> --- a/dlls/winealsa.drv/midi.c
> +++ b/dlls/winealsa.drv/midi.c
> @@ -869,7 +869,7 @@ static DWORD modData(WORD wDevID, DWORD dwParam)
>              snd_seq_ev_clear(&event);
>              snd_seq_ev_set_direct(&event);
>              snd_seq_ev_set_source(&event, port_out);
> -
>        snd_seq_ev_set_subs(&event);
> -
>        snd_seq_ev_set_dest(&event, MidiOutDev[wDevID].addr.client, MidiOutDev[wDevID].addr.port);
>
>  switch (evt & 0xF0) {
>  case MIDI_CMD_NOTE_OFF:
>
> @@ -1038,7 +1038,8 @@ static DWORD modLongData(WORD wDevID, LPMIDIHDR lpMidiHdr, DWORD dwSize)
>    snd_seq_ev_clear(&event);
>    snd_seq_ev_set_direct(&event);
>    snd_seq_ev_set_source(&event, port_out);
> - snd_seq_ev_set_subs(&event);
> - snd_seq_ev_set_dest(&event, MidiOutDev[wDevID].addr.client, MidiOutDev[wDevID].addr.port);
> - TRACE("destination %d:%d\n", MidiOutDev[wDevID].addr.client, MidiOutDev[wDevID].addr.port);
> snd_seq_ev_set_sysex(&event, lpMidiHdr->dwBufferLength + len_add, lpNewData ? lpNewData : lpData);
>     EnterCriticalSection(&midiSeqLock);
> snd_seq_event_output_direct(midiSeq, &event);
>
> -- 
> 2.32.0

[Jacek Konieczny]

On 12/16/21 5:25 PM, Andrew Eikum wrote:
> On Thu, Dec 16, 2021 at 11:21:11PM +0900, Masanori Kakura wrote:
> > This reverts commit 3d57cc2863f2f9a5ace40d29317b3ff4357fd119.
> >
> > Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52220
> > Signed-off-by: Masanori Kakura
>
> Thanks for reporting and looking into this, Kakurasan.
>
> Jacek, this patch was sent by you. Can you take a look at this bug and
> give us your thoughts?

I will have to investigate this. The point of my patch was to honour
port connections made both by the Windows application and by any
external applications. Reverting this would break the very use case I
wanted to fix.

But my patch was not supposed to break Windows applications even if they
try to use multiple ports. Either I misunderstood how the code was
previously worked or made some other mistake.

I'll look into it.

Greets,
Jacek

[Jacek Konieczny]

Hi Andrew,

On 12/16/21 5:25 PM, Andrew Eikum wrote:
> On Thu, Dec 16, 2021 at 11:21:11PM +0900, Masanori Kakura wrote:
> > This reverts commit 3d57cc2863f2f9a5ace40d29317b3ff4357fd119.
> >
> > Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52220
> > Signed-off-by: Masanori Kakura
>
> Thanks for reporting and looking into this, Kakurasan.
>
> Jacek, this patch was sent by you. Can you take a look at this bug and
> give us your thoughts?

I took the look and I wonder how I could miss it the previous time. I do
remember thinking about that and not finding it a problem.

Now it occurred to me that winealsa.drv would use a single output port
for all MidiOutDev devices. So the subscriptions indented for different
devices would all be added to the same port.

Below, there is a quick patch that I made, which makes a separate ALSA
port for every winealsa.drv output device. This way messages intended
for specific devices will only go there (by default) and Linux tools can
still route them differently.

If that approach is ok, I can prepare a proper patch for submission (but
the whitespace usage in the original code is still killing me).

The problem does not exist for input ports, as even when data from all
devices come to the single port the driver can still differentiate them
and route as needed internally.

While working on this I found something else weird going on with this
TMIDI program. Sometimes it would not send any MIDI messages at all or
send them only to one port – like the ports are not even properly
opened. This does not seem to be related to the changes made by me, though.

diff --git a/dlls/winealsa.drv/midi.c b/dlls/winealsa.drv/midi.c
index f2fe6307bb2..966cb458154 100644
--- a/dlls/winealsa.drv/midi.c
+++ b/dlls/winealsa.drv/midi.c
@@ -85,6 +85,7 @@ typedef struct {
     void*		lpExtra;	 	/* according to port type (MIDI, FM...), extra
data when needed */
     MIDIOUTCAPSW        caps;
     snd_seq_addr_t      addr;
+    int                 port_out;
 } WINE_MIDIOUT;

static WINE_MIDIIN	MidiInDev [MAX_MIDIINDRV ];
@@ -108,7 +109,6 @@ static	int		numOpenMidiSeq = 0;
 static	int		numStartedMidiIn = 0;

static int port_in;
-static int port_out;

static CRITICAL_SECTION crit_sect;   /* protects all MidiIn buffer
queues */
 static CRITICAL_SECTION_DEBUG critsect_debug =
@@ -247,21 +247,13 @@ static int midiOpenSeq(BOOL create_client)
             /* Setting the client name is the only init to do */
             snd_seq_set_client_name(midiSeq, "WINE midi driver");

-            port_out = snd_seq_create_simple_port(midiSeq, "WINE ALSA
Output",
-
SND_SEQ_PORT_CAP_READ|SND_SEQ_PORT_CAP_SUBS_READ|SND_SEQ_PORT_CAP_SUBS_WRITE,
-
SND_SEQ_PORT_TYPE_MIDI_GENERIC|SND_SEQ_PORT_TYPE_APPLICATION);
-            if (port_out < 0)
-                TRACE("Unable to create output port\n");
-            else
-                TRACE("Outport port %d created successfully\n", port_out);
-
             port_in = snd_seq_create_simple_port(midiSeq, "WINE ALSA
Input",

SND_SEQ_PORT_CAP_WRITE|SND_SEQ_PORT_CAP_SUBS_READ|SND_SEQ_PORT_CAP_SUBS_WRITE,

SND_SEQ_PORT_TYPE_MIDI_GENERIC|SND_SEQ_PORT_TYPE_APPLICATION);
             if (port_in < 0)
                 TRACE("Unable to create input port\n");
             else
-                TRACE("Input port %d created successfully\n", port_in);
+                TRACE("Input port :%d created successfully\n", port_in);
         }
     }
     numOpenMidiSeq++;
@@ -276,7 +268,6 @@ static int midiCloseSeq(void)
 {
     EnterCriticalSection(&midiSeqLock);
     if (--numOpenMidiSeq == 0) {
-	snd_seq_delete_simple_port(midiSeq, port_out);
    snd_seq_delete_simple_port(midiSeq, port_in);
    snd_seq_close(midiSeq);
    midiSeq = NULL;
@@ -738,6 +729,8 @@ static DWORD modGetDevCaps(WORD wDevID,
LPMIDIOUTCAPSW lpCaps, DWORD dwSize)
 static DWORD modOpen(WORD wDevID, LPMIDIOPENDESC lpDesc, DWORD dwFlags)
 {
     int ret;
+    int port_out;
+    char port_out_name[32];

TRACE("(%04X, %p, %08X);\n", wDevID, lpDesc, dwFlags);
     if (lpDesc == NULL) {
@@ -784,12 +777,30 @@ static DWORD modOpen(WORD wDevID, LPMIDIOPENDESC
lpDesc, DWORD dwFlags)
     MidiOutDev[wDevID].bufsize = 0x3FFF;
     MidiOutDev[wDevID].midiDesc = *lpDesc;

-    /* Connect our app port to the device port */
     EnterCriticalSection(&midiSeqLock);
-    ret = snd_seq_connect_to(midiSeq, port_out,
MidiOutDev[wDevID].addr.client,
-                             MidiOutDev[wDevID].addr.port);
+    /* Create a port dedicated to a specific device */
+    /* Keep the old name without a number for the first port */
+    if (wDevID)
+	sprintf(port_out_name, "WINE ALSA Output #%d", wDevID);
+    port_out = snd_seq_create_simple_port(midiSeq,
wDevID?port_out_name:"WINE ALSA Output",
+	
SND_SEQ_PORT_CAP_READ|SND_SEQ_PORT_CAP_SUBS_READ|SND_SEQ_PORT_CAP_SUBS_WRITE,
+	    SND_SEQ_PORT_TYPE_MIDI_GENERIC|SND_SEQ_PORT_TYPE_APPLICATION);
+
+    if (port_out < 0)
+	TRACE("Unable to create output port\n");
+    else {
+	TRACE("Outport port %d created successfully\n", port_out);
+
+	/* Connect our app port to the device port */
+	ret = snd_seq_connect_to(midiSeq, port_out,
MidiOutDev[wDevID].addr.client,
+	                         MidiOutDev[wDevID].addr.port);
+    }
     LeaveCriticalSection(&midiSeqLock);
-    if (ret < 0)
+
+    /* better have it set to error than a valid, but wrong value */
+    MidiOutDev[wDevID].port_out = port_out;
+
+    if (port_out < 0 || ret < 0)
    return MMSYSERR_NOTENABLED;

TRACE("Output port :%d connected
%d:%d\n",port_out,MidiOutDev[wDevID].addr.client,MidiOutDev[wDevID].addr.port);
@@ -825,7 +836,8 @@ static DWORD modClose(WORD wDevID)
     case MOD_MIDIPORT:
     case MOD_SYNTH:
         EnterCriticalSection(&midiSeqLock);
-        snd_seq_disconnect_to(midiSeq, port_out,
MidiOutDev[wDevID].addr.client, MidiOutDev[wDevID].addr.port);
+        TRACE("Deleting port :%d, connected to %d:%d\n",
MidiOutDev[wDevID].port_out, MidiOutDev[wDevID].addr.client,
MidiOutDev[wDevID].addr.port);
+        snd_seq_delete_simple_port(midiSeq, MidiOutDev[wDevID].port_out);
         LeaveCriticalSection(&midiSeqLock);
    midiCloseSeq();
    break;
@@ -870,7 +882,7 @@ static DWORD modData(WORD wDevID, DWORD dwParam)
             snd_seq_event_t event;
             snd_seq_ev_clear(&event);
             snd_seq_ev_set_direct(&event);
-            snd_seq_ev_set_source(&event, port_out);
+            snd_seq_ev_set_source(&event, MidiOutDev[wDevID].port_out);
             snd_seq_ev_set_subs(&event);
    
        switch (evt & 0xF0) {
@@ -1039,7 +1051,7 @@ static DWORD modLongData(WORD wDevID, LPMIDIHDR
lpMidiHdr, DWORD dwSize)
         }
    snd_seq_ev_clear(&event);
    snd_seq_ev_set_direct(&event);
-	snd_seq_ev_set_source(&event, port_out);
+	snd_seq_ev_set_source(&event, MidiOutDev[wDevID].port_out);
    snd_seq_ev_set_subs(&event);
    snd_seq_ev_set_sysex(&event, lpMidiHdr->dwBufferLength + len_add,
lpNewData ? lpNewData : lpData);
         EnterCriticalSection(&midiSeqLock);

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=104180

Your paranoid android.

=== debian11 (build log) ===

error: corrupt patch at line 45
Task: Patch failed to apply

=== debian11 (build log) ===

error: corrupt patch at line 45
Task: Patch failed to apply