Fix PTZ action commands getting ignored - #322
Conversation
5516761 to
4992717
Compare
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Switching to the prochandler API broke the action source because the proc handler lambdas were ignoring all calls from threads other than the GUI thread. This was done because the calldata structure usually lives on the callers stack, and if a call was queued on the correct thread, e.g. by calling QMetaObject::invokeMethod(), then the calldata would be gone by the time the method ran. However, the action source triggers don't come from the UI thread. To fix this, rework the proc handlers to extract the calldata arguments in the callers thread, and then use invokeMethod() to call the device method, using the default Qt::AutoConnection mode so that calls from the GUI thread are direct, but calls from other threads get queued. For most methods this works fine because the calls are commands to the device. The one exception is the ::get() method which needs to return data to the caller. This is the one proc handler method that still needs to be called by the GUI thread, and it gets ignored if it is not. This perhaps could be fixed by using a BlockingQueued connection, but it is unclear if it is needed and that change can be done in another patch. Fixes #321 Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
4992717 to
266134a
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses PTZ action/proc_handler commands being ignored when invoked from non-GUI threads by extracting calldata_t arguments on the caller thread and then dispatching device operations onto the device’s Qt thread via QMetaObject::invokeMethod(Qt::AutoConnection).
Changes:
- Removed the “GUI-thread-only” proc_handler restriction and updated PTZDevice proc handlers to queue device actions safely after copying
calldata_tinputs. - Made PTZDevice movement/preset/set handlers invoke device slots via
QMetaObject::invokeMethod()to support calls originating from non-UI threads. - Added thread-guard logging for
get()(still requiring same-thread execution) and updated a few tuple accessors tostd::get.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/settings.cpp | Uses std::get for tuple access in hotkey enumeration callbacks. |
| src/ptz-controls.cpp | Uses std::get for tuple access in hotkey lookup callback. |
| src/ptz-device.hpp | Promotes movement APIs to Qt slots and updates calldata wrappers to queue work via invokeMethod(). |
| src/ptz-device.cpp | Removes GUI-thread rejection in proc handler lambda; queues movement/preset/set operations via invokeMethod(); keeps get() thread-restricted. |
| src/ptz-visca.cpp | Adds thread guards to VISCA get()/set() (but set() currently reintroduces cross-thread ignoring). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #define ptz_ph_lambda(_method) [](void *p, calldata_t *cd) \ | ||
| { \ | ||
| auto ptz = static_cast<PTZDevice *>(p); \ | ||
| if (!ptz) { \ | ||
| blog(LOG_ERROR, "PTZ proc_handler called without PTZDevice pointer"); \ |
| if (QThread::currentThread() != thread()) { | ||
| ptz_log(LOG_ERROR, "PTZVisca::set(calldata) called from non-GUI thread; ignored"); | ||
| return; | ||
| } |
| void PTZDevice::get(calldata_t *cd) const | ||
| { | ||
| if (QThread::currentThread() != thread()) { | ||
| ptz_log(LOG_ERROR, "PTZDevice::get(calldata) called from non-GUI thread; ignored"); |
| void PTZVisca::get(calldata_t *cd) const | ||
| { | ||
| if (QThread::currentThread() != thread()) { | ||
| ptz_log(LOG_ERROR, "PTZVisca::get(calldata) called from non-GUI thread; ignored"); |
|
Reviewed this against the Copilot notes; two of them look worth acting on before merge. 1. void PTZVisca::set(calldata_t *cd)
{
- if (QThread::currentThread() != thread()) {
- ptz_log(LOG_ERROR, "PTZVisca::set(calldata) called from non-GUI thread; ignored");
- return;
- }
bool power_on;
if (calldata_get_bool(cd, "power_on", &power_on))
- send(VISCA_CAM_Power, {power_on});
+ QMetaObject::invokeMethod(this, [this, power_on]() { send(VISCA_CAM_Power, {power_on}); });
long long wb_mode;
if (calldata_get_int(cd, "wb_mode", &wb_mode))
- send(VISCA_CAM_WB_Mode, {(int)wb_mode});
+ QMetaObject::invokeMethod(this, [this, wb_mode]() { send(VISCA_CAM_WB_Mode, {(int)wb_mode}); });
bool trigger;
if (calldata_get_bool(cd, "wb_onepush_trigger", &trigger) && trigger)
- send(VISCA_CAM_WB_OnePushTrigger);
+ QMetaObject::invokeMethod(this, [this]() { send(VISCA_CAM_WB_OnePushTrigger); });
PTZDevice::set(cd);
}The functor overload of 2. The block comment above The two low-severity notes are fair as well: the remaining checks compare against |
Switching to the prochandler API broke the action source because the proc handler lambdas were ignoring all calls from threads other than the GUI thread. This was done because the calldata structure usually lives on the callers stack, and if a call was queued on the correct thread, e.g. by calling QMetaObject::invokeMethod(), then the calldata would be gone by the time the method ran. However, the action source triggers don't come from the UI thread.
To fix this, rework the proc handlers to extract the calldata arguments in the callers thread, and then use invokeMethod() to call the device method, using the default Qt::AutoConnection mode so that calls from the GUI thread are direct, but calls from other threads get queued. For most methods this works fine because the calls are commands to the device.
The one exception is the ::get() method which needs to return data to the caller. This is the one proc handler method that still needs to be called by the GUI thread, and it gets ignored if it is not. This perhaps could be fixed by using a BlockingQueued connection, but it is unclear if it is needed and that change can be done in another patch.