search

ak5k / reablink

REAPER plug-in extension providing ReaScript bindings for Ableton Link session, and Ableton Link Test Plan compliant implementations for REAPER.
MIT License
39 stars 0 forks source link

Unsafe function call OnStopButton #2

Closed helgoboss closed 1 year ago

helgoboss commented 1 year ago

Hi @ak5k! A ReaLearn user reported a "conflict" with ReaBlink (https://github.com/helgoboss/realearn/issues/705). I investigated a bit and saw that ReaBlink calls REAPER's OnStopButton function from the wrong thread. Most REAPER functions must be called from the main thread only. If not, it's undefined behavior (crash, weird effects, ...). I will double check with Justin just to be sure but I think OnStopButton is no exception.

You can fix this by deferring the execution of OnStopButton to the main thread (e.g. via a non-blocking queue).

BTW, this could very well be the cause for #1. ReaLearn detected this incorrect state because it handles REAPER callbacks that occur for example when the play state changes.

ak5k commented 1 year ago

Hi! Does this happen with 0.5.0-rc4 ? It has other issues, but it shouldnt call functions directly from audio thread.

helgoboss commented 1 year ago

The user who reported this mentioned he built ReaBlink directly from GitHub, so I assume he used the latest code.

The code which calls REAPER functions from the wrong thread is definitely still present in the main branch tip: https://github.com/ak5k/reablink/blob/main/source/BlinkEngine.cpp#L336. Yes, it doesn't call from the audio thread, but from a custom worker thread. I got confirmation from Justin that calling OnStopButton is no exception. It must only be called from the main thread.

It doesn't seem to be the only function call with that issue: SetEditCurPos, Main_OnCommand, OnPlayButton, SetTempo, UpdateTimeline ... all main-thread only.

I'm unsure about SetTempoTimeSigMarker, GetTempoTimeSigMarker and FindTempoTimeSigMarker.

TimeMap_timeToQN_abs, TimeMap_GetTimeSigAtTime is okay to call from any thread, AFAIK.

ak5k commented 1 year ago

Hmm, so at this point would probably make more sense just completely re-implement the Puppet mode into a deferred Lua script.