seq: chrome web-MIDI issues since 1.2.10

[nodens]

Hi,

not sure how to qualify this issue exactly, it's probably unrelated to web MIDI, but that's how I can test and reproduce it in my setup.

I own an Expert Sleepers ES-9 interface that uses a web MIDI interface for its configuration (https://www.expert-sleepers.co.uk/webapps/es9_config_tool_1.3.html). The page send and receive sysex messages from the device.

Since 1.2.10, it seems to only receive (or send) a fraction of the messages. I can sometimes (not every time) get the result of a sample rate request, but there is never any response on configuration dump request or version request. using aseqdump to see the messages shows nothing: there is no message seen at all.

Downgrading alsa-lib to 1.2.9 resolves the issue. I'm not sure how to debug further, but it is plausibly related to the recent seq changes.

Just to be sure, I also tried with latest HEAD, which doesn't change the behaviour.

Cheers,

nodens

[perexg]

Thanks for your report. Could you try to git bisect to the commit causing this issue ?

[nodens]

I was just doing that :) here is the result:

2aefb5c41cc03b72b3161b57336dd4f924b7a90b is the first bad commit
commit 2aefb5c41cc03b72b3161b57336dd4f924b7a90b
Author: Takashi Iwai <tiwai@suse.de>
Date:   Thu Nov 17 15:49:44 2022 +0100

    seq: Add UMP support

    This patch adds the basic support of UMP on ALSA sequencer API.
    An extended event type, snd_seq_ump_event_t, is defined.  It's
    compatible with the existing type, snd_seq_event_t, but it has a
    larger payload of 16 bytes instead of 12 bytes, for holding the full
    128bit UMP packet.

    The new snd_seq_ump_event_t must have the bit SND_SEQ_EVENT_UMP in the
    event flags.

    A few new API functions have been added such as
    snd_seq_ump_event_output() and snd_seq_ump_event_input() for
    reading/writing this new event object.

    The support of UMP in the sequencer client is switched by the function
    snd_seq_client_set_midi_version().  It can switch from the default
    legacy MIDI to UMP MIDI 1.0 or 2.0 on the fly.

    The automatic event conversion among UMP and legacy clients can be
    suppressed via snd_seq_client_set_ump_conversion().

    The inquiry of the associated UMP Endpoints and UMP Blocks can be done
    via snd_seq_get_ump_endpoint_info() and snd_seq_get_ump_block_info().

    Signed-off-by: Takashi Iwai <tiwai@suse.de>

 include/local.h     |   2 +
 include/seq.h       |  44 ++++++
 include/seq_event.h |  42 ++++--
 include/seqmid.h    |  24 ++++
 src/Versions.in     |  17 +++
 src/seq/seq.c       | 404 +++++++++++++++++++++++++++++++++++++++++++++++-----
 src/seq/seq_hw.c    |  72 +++++++++-
 src/seq/seq_local.h |   6 +-
 src/seq/seqmid.c    |  38 +++++
 9 files changed, 601 insertions(+), 48 deletions(-)

[tiwai]

The commit needs at least the follow-up commit dc1e683cc2d2aaed938f9459605d51d4a2cb7ba0

But I guess you're still hitting even if you apply the fix?

Which kernel version are you using? With the older kernel (before 6.5), or a kernel with UMP support (6.5 or later)?

[nodens]

Kernel version is current Debian Unstable kernel: 6.5.0-3-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.5.8-1 (2023-10-22) x86_64 GNU/Linux So I guess with UMP support, but I see # CONFIG_SND_SEQ_UMP is not set in the config.gz.

I tried to checkout 2aefb5c41cc03b72b3161b57336dd4f924b7a90b and then cherry-picked https://github.com/alsa-project/alsa-lib/commit/dc1e683cc2d2aaed938f9459605d51d4a2cb7ba0 : I still have the issue. Not sure if that was the right way to test, though ? My git-fu might be a bit weak ;)

[tiwai]

Could you try to fix like below on the top? It's an obvious error, at least.

--- a/src/seq/seq.c
+++ b/src/seq/seq.c
@@ -1269,9 +1269,9 @@ int snd_seq_set_input_buffer_size(snd_seq_t *seq, size_t size)
    size_t packet_size;

    assert(seq && seq->ibuf);
+   packet_size = get_packet_size(seq);
    assert(size >= packet_size);
    snd_seq_drop_input(seq);
-   packet_size = get_packet_size(seq);
    size = (size + packet_size - 1) / packet_size;
    if (size != seq->ibufsize) {
        char *newbuf;

[nodens]

I tried (both on top of my test branch with the commits above and on top of HEAD), but I still have the same behaviour: no response from the interface when the web app tries to send a sysex.

Damn, that looked like a good candidate ;)

[tiwai]

Also check which value is received from info->midi_version at update_midi_version() in src/seq/seq_hw.c (e.g. by adding a debug print). This should be 0 in your case.

[nodens]

I might have messed up somewhere, but it's null rather than 0:

ALSA lib seq_hw.c:134:(snd_seq_hw_set_client_info) midi version: (null) (it actually appear twice in a row)

My changes on top of HEAD being:

diff --git a/src/seq/seq.c b/src/seq/seq.c
index fd8ca30e..5ec737a7 100644
--- a/src/seq/seq.c
+++ b/src/seq/seq.c
@@ -1269,9 +1269,9 @@ int snd_seq_set_input_buffer_size(snd_seq_t *seq, size_t size)
        size_t packet_size;

        assert(seq && seq->ibuf);
+       packet_size = get_packet_size(seq);
        assert(size >= packet_size);
        snd_seq_drop_input(seq);
-       packet_size = get_packet_size(seq);
        size = (size + packet_size - 1) / packet_size;
        if (size != seq->ibufsize) {
                char *newbuf;
diff --git a/src/seq/seq_hw.c b/src/seq/seq_hw.c
index b74948c8..e674eace 100644
--- a/src/seq/seq_hw.c
+++ b/src/seq/seq_hw.c
@@ -131,6 +131,7 @@ static int snd_seq_hw_set_client_info(snd_seq_t *seq, snd_seq_client_info_t * in
                return -errno;
        }
        update_midi_version(seq, info);
+  SYSERR("midi version: %s", info->midi_version);
        return 0;
 }

(I'm not a C person… Or even a real developer, more on the system side).

[tiwai]

You should use %d instead of %s. But I suppose it's 0.

And, this is called only once at it's 0, or it's called multiple times and always 0?

[tiwai]

Try to put the following:

--- a/src/seq/seq_hw.c
+++ b/src/seq/seq_hw.c
@@ -535,6 +535,7 @@ int snd_seq_hw_open(snd_seq_t **handle, const char *name, int streams, int mode)
        close(fd);
        return ret;
    }
+   ver = SNDRV_PROTOCOL_VERSION(1, 0, 2); // XXX
    if (SNDRV_PROTOCOL_INCOMPATIBLE(ver, SNDRV_SEQ_VERSION_MAX)) {
        close(fd);
        return -SND_ERROR_INCOMPATIBLE_VERSION;

This should enforce the old behavior in the library side.

[nodens]

You should use %d instead of %s. But I suppose it's 0.

Duh. Of course. Sorry 😅

And, this is called only once at it's 0, or it's called multiple times and always 0?

it's called twice, and always 0.

This should enforce the old behavior in the library side.

I get the same behaviour with this change, no answer from the interface :/ Here is my diff against current HEAD:

diff --git a/src/seq/seq_hw.c b/src/seq/seq_hw.c
index b74948c8..9165a18a 100644
--- a/src/seq/seq_hw.c
+++ b/src/seq/seq_hw.c
@@ -131,6 +131,7 @@ static int snd_seq_hw_set_client_info(snd_seq_t *seq, snd_seq_client_info_t * in
                return -errno;
        }
        update_midi_version(seq, info);
+       SYSERR("midi version: %d", info->midi_version);
        return 0;
 }

@@ -535,6 +536,7 @@ int snd_seq_hw_open(snd_seq_t **handle, const char *name, int streams, int mode)
                close(fd);
                return ret;
        }
+       ver = SNDRV_PROTOCOL_VERSION(1, 0, 2); // enforce old behaviour - from #360
        if (SNDRV_PROTOCOL_INCOMPATIBLE(ver, SNDRV_SEQ_VERSION_MAX)) {
                close(fd);
                return -SND_ERROR_INCOMPATIBLE_VERSION;

[tiwai]

OK, then could you try alsa-lib 1.2.10 (keeping the modifications) with 6.4 or older kernel? Just to make sure that it's a problem of alsa-lib, not about kernel.

[nodens]

Sure. No difference in behaviour with 6.4.0-4-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.4.13-1 (2023-08-31) x86_64 GNU/Linux

[tiwai]

Thanks. Then it's an issue with user-space. It seems that Chrome sends events with incorrect flag bits indicating as if it were a UMP event.

Could you try the patch below? seq-ump-legacy-apps-workaround.diff.txt

[nodens]

Yes! That works for me, looks like this is indeed the issue. Thanks!

[tiwai]

Good to hear! I'll push the fix commit and a few more cleanup patches.