Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LV2: program_changed call deadlocks Carla #1968

Open
kramlie opened this issue Jan 31, 2025 · 2 comments
Open

LV2: program_changed call deadlocks Carla #1968

kramlie opened this issue Jan 31, 2025 · 2 comments

Comments

@kramlie
Copy link

kramlie commented Jan 31, 2025

I have only witnessed this while developing features for Yoshimi, so I'm not sure if this is reproducible in the wild yet. However, you don't really need to reproduce the issue to see what the problem is.

If the LV2 plugin calls program_changed with -1, it triggers a full reload of all programs. This happens in handleProgramChanged. Inside of reloadPrograms, setMidiProgram will usually be called. The problem is that setMidiProgram will attempt to use ScopedSingleProcessLocker, but handleProgramChanged already holds this lock, leading to a deadlock.

I would have submitted a pull request for this myself, but I'm not completely sure how to handle this situation, since the setMidiProgram is used from multiple places, so the lock can't just be removed. reloadPrograms is also used from multiple places, and only one place has the lock. So I thought I'd ask here first.

We could potentially pass in a bool to signal whether we are already holding the lock or not. Other ideas?

@falkTX
Copy link
Owner

falkTX commented Jan 31, 2025

if only deadlocks if you dont follow the threading rules around this API
https://github.com/KXStudio/LV2-Extensions/blob/master/kx-programs.lv2/programs.h#L160

@kramlie
Copy link
Author

kramlie commented Feb 1, 2025

@falkTX: Yes, I already hit that one 😄, and that mistake is on me.

But this is a different deadlock. I have fixed the issue with calling from RT context, and I'm certain I'm calling it from the right thread now (a low prio worker thread).

If we take a look at this backtrace (see inline comments in uppercase):

#0  __lll_clocklock_wait (futex=futex@entry=0x7fffdfa9d974, val=val@entry=0, clockid=clockid@entry=0, abstime=abstime@entry=0x0, private=private@entry=0) at lll_timedlock_wait.c:59
#1  0x00007ffff7d34ea1 in __pthread_mutex_lock_full (mutex=0x5fb71d8) at ../nptl/pthread_mutex_lock.c:432
#2  0x00007fffeeab3984 in CarlaMutex::lock() const (this=0x5fb71d8) at ../../utils/CarlaMutex.hpp:71
#3  0x00007fffeeb0e21f in CarlaBackend::CarlaPlugin::ScopedSingleProcessLocker::ScopedSingleProcessLocker(CarlaBackend::CarlaPlugin*, bool) (this=0x7fffdfa9da50, plugin=0x5efcbd0, block=true) at CarlaPlugin.cpp:2715
#4  0x00007fffeeb4a89f in CarlaBackend::CarlaPluginLV2::setMidiProgram(int, bool, bool, bool, bool) (this=0x5efcbd0, index=0, sendGui=true, sendOsc=true, sendCallback=true, doingInit=false) at CarlaPluginLV2.cpp:1734
    ^--- ATTEMPTS TO GRAB THE SAME LOCK HERE
#5  0x00007fffeeb52d7a in CarlaBackend::CarlaPluginLV2::reloadPrograms(bool) (this=0x5efcbd0, doInit=false) at CarlaPluginLV2.cpp:3544
#6  0x00007fffeeb5be2d in CarlaBackend::CarlaPluginLV2::handleProgramChanged(int) (this=0x5efcbd0, index=-1) at CarlaPluginLV2.cpp:5762
    ^--- LOCK IS GRABBED HERE
#7  0x00007fffeeb62544 in CarlaBackend::CarlaPluginLV2::carla_lv2_program_changed(void*, int) (handle=0x5efcbd0, index=-1) at CarlaPluginLV2.cpp:7688
#8  0x00007fffec0ba47c in YoshimiLV2Plugin::<lambda()>::operator()(void) const (__closure=0x64b6498) at ~/code/yoshimi/src/LV2_Plugin/YoshimiLV2Plugin.cpp:397
#9  ...

We can see this is all happening in one single thread. At least to my eyes it looks like it will always deadlock if it calls setMidiProgram, which happens whenever there is a change in the number of programs. Or am I missing something obvious here..?

kramlie added a commit to kramlie/yoshimi that referenced this issue Feb 3, 2025
Turns out that the conditional program list population introduced in
7ce151b is problematic. The issue was discovered while working
on making program list changes dynamic (inform the host of list
changes [1]). Take for example this scenario:

  1. In the `~/.config/yoshimi/yoshimi-LV2.instance` file,
     `ignore_program_changes` is true.

  2. In the LV2 state data, the same option is set to false (so
     Program Changes are enabled).

  3. Because programs are queried before the state is loaded, this
     results in an empty list initially, because of point 1.

  4. Then the state is loaded, which causes the list to be
     populated again.

The problem is that some hosts have trouble with the window between
step 3 and 4, and will often forget which program was set while the
list is empty. Then they will set it to a default one once the list is
populated again, instead of the one which they had saved earlier. This
was only tested on Carla, but it's reasonable to assume that other
hosts may have similar problems.

Therefore, always populate the list with all entries, so that it's
stable. However, we will only respond to program changes that can be
made using the current combination of root, bank and program change
options. The root and bank LSB/MSB settings are not significant, but
they do need to be on to have any effect. Programs that do not match
this "options filter" are ignored.

[1] Implementing dynamic program list changes was abandoned because
    host support seems quite poor (at least in Carla), and in fact
    introduced more problems than it seemingly solved:

    * Carla deadlocks when programs are changed (see
      falkTX/Carla#1968)

    * Its program selection is nonsensical when programs change (see
      releadPrograms in Carla's CarlaPluginLV2.cpp).

    * Carla doesn't even save the program you have selected, so it's
      kind of useless other than to just select one as a one-off.

Signed-off-by: Kristian Amlie <[email protected]>
kramlie added a commit to kramlie/yoshimi that referenced this issue Feb 3, 2025
Turns out that the conditional program list population introduced in
7ce151b is problematic. The issue was discovered while working
on making program list changes dynamic (inform the host of list
changes [1]). Take for example this scenario:

  1. In the `~/.config/yoshimi/yoshimi-LV2.instance` file,
     `ignore_program_changes` is true.

  2. In the LV2 state data, the same option is set to false (so
     Program Changes are enabled).

  3. Because programs are queried before the state is loaded, this
     results in an empty list initially, because of point 1.

  4. Then the state is loaded, which causes the list to be
     populated again.

The problem is that some hosts have trouble with the window between
step 3 and 4, and will often forget which program was set while the
list is empty. Then they will set it to a default one once the list is
populated again, instead of the one which they had saved earlier. This
was only tested on Carla, but it's reasonable to assume that other
hosts may have similar problems.

Therefore, always populate the list with all entries, so that it's
stable. However, we will only respond to program changes that can be
made using the current combination of root, bank and program change
options. The root and bank LSB/MSB settings are not significant, but
they do need to be on to have any effect. Programs that do not match
this "options filter" are ignored.

[1] Implementing dynamic program list changes was abandoned because
    host support seems quite poor (at least in Carla), and in fact
    introduced more problems than it seemingly solved:

    * Carla deadlocks when programs are changed (see
      falkTX/Carla#1968)

    * Its program selection is nonsensical when programs change (see
      releadPrograms in Carla's CarlaPluginLV2.cpp).

    * Carla doesn't even save the program you have selected, so it's
      kind of useless other than to just select one as a one-off.

Signed-off-by: Kristian Amlie <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants