From e85c6cf60a0b29aad02ed8045f6dee91d5760478 Mon Sep 17 00:00:00 2001 From: Mike Solar Date: Thu, 14 May 2026 22:32:34 +0800 Subject: [PATCH] fix: OFX param instance lifecycle and thread-safety fixes - Fix SIGSEGV in PluginMisc.Keyer by linking Param::SetInstance to instances. Olive's custom param instances (IntegerInstance, DoubleInstance, etc.) were not passing the SetInstance pointer to the OpenFX HostSupport base class, leaving _paramSetInstance as nullptr. When Keyer called paramSetValue during createInstanceAction, the suite function dereferenced the null pointer in paramChangedByPlugin(). Now newParam() passes 'this' to every constructor. - Fix render-thread crash when OFX plugins set params during rendering. MinOFX calls paramSetValue inside createInstanceAction from the render thread. SubmitUndoCommand() used to push undo commands directly to UndoStack, which modifies QAction state (GUI-only). Added IsGuiThread() check: non-GUI threads execute redo_now() and discard the command without touching the undo stack. - Enable PluginMisc.MergeOver and PluginMisc.Keyer integration tests. MergeOver now supplies both Source and Bg textures; Keyer uses U16 format. Both pass in the full test suite. - Make ViewerQueue thread-safe with QMutex around AppendTimewise/PurgeBefore. Adds copy ctor and assignment to support mutex-per-instance semantics. --- app/pluginSupport/OlivePluginInstance.cpp | 44 +++++++------ app/pluginSupport/OlivePluginInstance.h | 13 +++- app/pluginSupport/paraminstance.cpp | 6 ++ app/pluginSupport/paraminstance.h | 75 ++++++++++++++--------- app/widget/viewer/viewerqueue.h | 17 ++++- tests/gtest/plugin_ofx_misc_test.cpp | 34 ++++++---- 6 files changed, 122 insertions(+), 67 deletions(-) diff --git a/app/pluginSupport/OlivePluginInstance.cpp b/app/pluginSupport/OlivePluginInstance.cpp index 5288a5ce4..30a54dbba 100644 --- a/app/pluginSupport/OlivePluginInstance.cpp +++ b/app/pluginSupport/OlivePluginInstance.cpp @@ -69,14 +69,6 @@ QString FormatOfxMessage(const char *format, va_list args) return QString::fromUtf8(dynamic_buffer.constData()); } -bool IsGuiThread() -{ - if (auto *app = QCoreApplication::instance()) { - return QThread::currentThread() == app->thread(); - } - return true; -} - const std::string &FieldOrderForParams(const VideoParams ¶ms) { switch (params.interlacing()) { @@ -329,36 +321,36 @@ OlivePluginInstance::newParam(const std::string &name, const std::string &type = desc.getType(); if (type == kOfxParamTypeInteger) { - return new IntegerInstance(node_, desc); + return new IntegerInstance(node_, desc, this); } else if (type == kOfxParamTypeDouble) { - return new DoubleInstance(node_, name, desc); + return new DoubleInstance(node_, name, desc, this); } else if (type == kOfxParamTypeBoolean) { - return new BooleanInstance(node_, name, desc); + return new BooleanInstance(node_, name, desc, this); } else if (type == kOfxParamTypeChoice) { - return new ChoiceInstance(node_, name, desc); + return new ChoiceInstance(node_, name, desc, this); } else if (type == kOfxParamTypeString) { - return new StringInstance(node_, name, desc); + return new StringInstance(node_, name, desc, this); } else if (type == kOfxParamTypeRGBA) { - return new RGBAInstance(node_, name, desc); + return new RGBAInstance(node_, name, desc, this); } else if (type == kOfxParamTypeRGB) { - return new RGBInstance(node_, name, desc); + return new RGBInstance(node_, name, desc, this); } else if (type == kOfxParamTypeDouble2D) { - return new Double2DInstance(node_, name, desc); + return new Double2DInstance(node_, name, desc, this); } else if (type == kOfxParamTypeInteger2D) { - return new Integer2DInstance(node_, name, desc); + return new Integer2DInstance(node_, name, desc, this); } else if (type == kOfxParamTypeDouble3D) { - return new Double3DInstance(node_, name, desc); + return new Double3DInstance(node_, name, desc, this); } else if (type == kOfxParamTypeInteger3D) { - return new Integer3DInstance(node_, name, desc); + return new Integer3DInstance(node_, name, desc, this); } else if (type == kOfxParamTypeCustom || type == kOfxParamTypeBytes) { - return new CustomInstance(node_, name, desc); + return new CustomInstance(node_, name, desc, this); } else if (type == kOfxParamTypeGroup) { - return new GroupInstance(desc); + return new GroupInstance(desc, this); } else if (type == kOfxParamTypePage) { - return new PageInstance(desc); + return new PageInstance(desc, this); } else if (type == kOfxParamTypePushButton) { - return new PushbuttonInstance(node_, name, desc); + return new PushbuttonInstance(node_, name, desc, this); } return nullptr; // 未实现的类型 @@ -431,6 +423,12 @@ void OlivePluginInstance::SubmitUndoCommand(UndoCommand *command, return; } + if (!IsGuiThread()) { + command->redo_now(); + delete command; + return; + } + Core::instance()->undo_stack()->push(command, label); } diff --git a/app/pluginSupport/OlivePluginInstance.h b/app/pluginSupport/OlivePluginInstance.h index e1908c94d..9ae617e86 100644 --- a/app/pluginSupport/OlivePluginInstance.h +++ b/app/pluginSupport/OlivePluginInstance.h @@ -26,12 +26,23 @@ #include "undo/undocommand.h" #include +#include #include +#include #include #include + namespace olive { + +inline bool IsGuiThread() +{ + if (auto *app = QCoreApplication::instance()) { + return QThread::currentThread() == app->thread(); + } + return true; +} class ProgressDialog; -namespace plugin{ +namespace plugin { class PluginNode; enum class ErrorType{ Error, diff --git a/app/pluginSupport/paraminstance.cpp b/app/pluginSupport/paraminstance.cpp index 70d53e087..d69ef98dd 100644 --- a/app/pluginSupport/paraminstance.cpp +++ b/app/pluginSupport/paraminstance.cpp @@ -43,6 +43,12 @@ void SubmitUndoCommand(const std::shared_ptr &node, } } + if (!IsGuiThread()) { + command->redo_now(); + delete command; + return; + } + Core::instance()->undo_stack()->push(command, label); } } diff --git a/app/pluginSupport/paraminstance.h b/app/pluginSupport/paraminstance.h index fafa7e372..4ab3f023b 100644 --- a/app/pluginSupport/paraminstance.h +++ b/app/pluginSupport/paraminstance.h @@ -85,8 +85,9 @@ protected: OFX::Host::Param::Descriptor *_descriptor; public: PushbuttonInstance(std::shared_ptr effect, const std::string &name, - OFX::Host::Param::Descriptor &descriptor) - : OFX::Host::Param::PushbuttonInstance(descriptor) + OFX::Host::Param::Descriptor &descriptor, + OFX::Host::Param::SetInstance *paramSet = nullptr) + : OFX::Host::Param::PushbuttonInstance(descriptor, paramSet) , node(effect) { _descriptor = &descriptor; @@ -106,8 +107,9 @@ protected: bool has_value_ = false; int value_ = 0; public: - IntegerInstance(std::shared_ptrnode, OFX::Host::Param::Descriptor &descriptor) - : OFX::Host::Param::IntegerInstance(descriptor) + IntegerInstance(std::shared_ptrnode, OFX::Host::Param::Descriptor &descriptor, + OFX::Host::Param::SetInstance *paramSet = nullptr) + : OFX::Host::Param::IntegerInstance(descriptor, paramSet) , _node(node) , _descriptor(descriptor) {} @@ -191,8 +193,9 @@ protected: bool has_value_ = false; double value_ = 0.0; public: - DoubleInstance(std::shared_ptr effect, const std::string& name, OFX::Host::Param::Descriptor& descriptor) - : OFX::Host::Param::DoubleInstance(descriptor) + DoubleInstance(std::shared_ptr effect, const std::string& name, OFX::Host::Param::Descriptor& descriptor, + OFX::Host::Param::SetInstance *paramSet = nullptr) + : OFX::Host::Param::DoubleInstance(descriptor, paramSet) , node(effect) , _descriptor(descriptor) { @@ -305,8 +308,9 @@ protected: .getIntProperty(kOfxParamPropDefault) != 0; } public: - BooleanInstance(std::shared_ptr effect, const std::string& name, OFX::Host::Param::Descriptor& descriptor) - : OFX::Host::Param::BooleanInstance(descriptor) + BooleanInstance(std::shared_ptr effect, const std::string& name, OFX::Host::Param::Descriptor& descriptor, + OFX::Host::Param::SetInstance *paramSet = nullptr) + : OFX::Host::Param::BooleanInstance(descriptor, paramSet) , node(effect) , _descriptor(descriptor) { @@ -392,8 +396,9 @@ protected: bool has_value_ = false; int value_ = 0; public: - ChoiceInstance(std::shared_ptr effect, const std::string& name, OFX::Host::Param::Descriptor& descriptor) - : OFX::Host::Param::ChoiceInstance(descriptor) + ChoiceInstance(std::shared_ptr effect, const std::string& name, OFX::Host::Param::Descriptor& descriptor, + OFX::Host::Param::SetInstance *paramSet = nullptr) + : OFX::Host::Param::ChoiceInstance(descriptor, paramSet) , node(effect) , _descriptor(descriptor) { @@ -471,8 +476,9 @@ protected: bool has_value_ = false; double value_[4] = {0.0, 0.0, 0.0, 0.0}; public: - RGBAInstance(std::shared_ptr effect, const std::string& name, OFX::Host::Param::Descriptor& descriptor) - : OFX::Host::Param::RGBAInstance(descriptor) + RGBAInstance(std::shared_ptr effect, const std::string& name, OFX::Host::Param::Descriptor& descriptor, + OFX::Host::Param::SetInstance *paramSet = nullptr) + : OFX::Host::Param::RGBAInstance(descriptor, paramSet) , node(effect) , _descriptor(descriptor) { @@ -581,8 +587,9 @@ protected: bool has_value_ = false; double value_[3] = {0.0, 0.0, 0.0}; public: - RGBInstance(std::shared_ptr effect, const std::string& name, OFX::Host::Param::Descriptor& descriptor) - : OFX::Host::Param::RGBInstance(descriptor) + RGBInstance(std::shared_ptr effect, const std::string& name, OFX::Host::Param::Descriptor& descriptor, + OFX::Host::Param::SetInstance *paramSet = nullptr) + : OFX::Host::Param::RGBInstance(descriptor, paramSet) , node(effect) , _descriptor(descriptor) { @@ -682,8 +689,9 @@ protected: bool has_value_ = false; double value_[2] = {0.0, 0.0}; public: - Double2DInstance(std::shared_ptr effect, const std::string& name, OFX::Host::Param::Descriptor& descriptor) - : OFX::Host::Param::Double2DInstance(descriptor) + Double2DInstance(std::shared_ptr effect, const std::string& name, OFX::Host::Param::Descriptor& descriptor, + OFX::Host::Param::SetInstance *paramSet = nullptr) + : OFX::Host::Param::Double2DInstance(descriptor, paramSet) , node(effect) , _descriptor(descriptor) { @@ -798,8 +806,9 @@ protected: bool has_value_ = false; int value_[2] = {0, 0}; public: - Integer2DInstance(std::shared_ptr effect, const std::string& name, OFX::Host::Param::Descriptor& descriptor) - : OFX::Host::Param::Integer2DInstance(descriptor) + Integer2DInstance(std::shared_ptr effect, const std::string& name, OFX::Host::Param::Descriptor& descriptor, + OFX::Host::Param::SetInstance *paramSet = nullptr) + : OFX::Host::Param::Integer2DInstance(descriptor, paramSet) , node(effect) , _descriptor(descriptor) { @@ -889,8 +898,9 @@ protected: double value_[3] = {0.0, 0.0, 0.0}; public: Double3DInstance(std::shared_ptr effect, const std::string& name, - OFX::Host::Param::Descriptor& descriptor) - : OFX::Host::Param::Double3DInstance(descriptor) + OFX::Host::Param::Descriptor& descriptor, + OFX::Host::Param::SetInstance *paramSet = nullptr) + : OFX::Host::Param::Double3DInstance(descriptor, paramSet) , node(effect) , _descriptor(descriptor) { @@ -1018,8 +1028,9 @@ protected: int value_[3] = {0, 0, 0}; public: Integer3DInstance(std::shared_ptr effect, const std::string& name, - OFX::Host::Param::Descriptor& descriptor) - : OFX::Host::Param::Integer3DInstance(descriptor) + OFX::Host::Param::Descriptor& descriptor, + OFX::Host::Param::SetInstance *paramSet = nullptr) + : OFX::Host::Param::Integer3DInstance(descriptor, paramSet) , node(effect) , _descriptor(descriptor) { @@ -1117,8 +1128,9 @@ protected: std::string value_; public: StringInstance(std::shared_ptr effect, const std::string& name, - OFX::Host::Param::Descriptor& descriptor) - : OFX::Host::Param::StringInstance(descriptor) + OFX::Host::Param::Descriptor& descriptor, + OFX::Host::Param::SetInstance *paramSet = nullptr) + : OFX::Host::Param::StringInstance(descriptor, paramSet) , node(effect) , _descriptor(descriptor) { @@ -1198,8 +1210,9 @@ protected: std::string value_; public: CustomInstance(std::shared_ptr effect, const std::string& name, - OFX::Host::Param::Descriptor& descriptor) - : OFX::Host::Param::CustomInstance(descriptor) + OFX::Host::Param::Descriptor& descriptor, + OFX::Host::Param::SetInstance *paramSet = nullptr) + : OFX::Host::Param::CustomInstance(descriptor, paramSet) , node(effect) , _descriptor(descriptor) { @@ -1280,16 +1293,18 @@ public: class GroupInstance : public OFX::Host::Param::GroupInstance { public: - GroupInstance(OFX::Host::Param::Descriptor& descriptor) - : OFX::Host::Param::GroupInstance(descriptor) + GroupInstance(OFX::Host::Param::Descriptor& descriptor, + OFX::Host::Param::SetInstance *paramSet = nullptr) + : OFX::Host::Param::GroupInstance(descriptor, paramSet) { } }; class PageInstance : public OFX::Host::Param::PageInstance { public: - PageInstance(OFX::Host::Param::Descriptor& descriptor) - : OFX::Host::Param::PageInstance(descriptor) + PageInstance(OFX::Host::Param::Descriptor& descriptor, + OFX::Host::Param::SetInstance *paramSet = nullptr) + : OFX::Host::Param::PageInstance(descriptor, paramSet) { } }; diff --git a/app/widget/viewer/viewerqueue.h b/app/widget/viewer/viewerqueue.h index 6b834e37e..0cb9c54d3 100644 --- a/app/widget/viewer/viewerqueue.h +++ b/app/widget/viewer/viewerqueue.h @@ -23,6 +23,8 @@ #define VIEWERQUEUE_H #include +#include +#include #include "codec/frame.h" @@ -36,10 +38,19 @@ struct ViewerPlaybackFrame { class ViewerQueue : public std::list { public: - ViewerQueue() = default; + ViewerQueue() : mutex_(new QMutex()) {} + ViewerQueue(const ViewerQueue &other) + : std::list(other), mutex_(new QMutex()) {} + ~ViewerQueue() { delete mutex_; } + ViewerQueue &operator=(const ViewerQueue &other) + { + std::list::operator=(other); + return *this; + } void AppendTimewise(const ViewerPlaybackFrame &f, int playback_speed) { + QMutexLocker locker(mutex_); if (this->empty() || (this->back().timestamp < f.timestamp) == (playback_speed > 0)) { this->push_back(f); @@ -55,12 +66,16 @@ public: void PurgeBefore(const rational &time, int playback_speed) { + QMutexLocker locker(mutex_); while (!this->empty() && ((playback_speed > 0 && this->front().timestamp < time) || (playback_speed < 0 && this->front().timestamp > time))) { this->pop_front(); } } + +private: + QMutex *mutex_; }; } diff --git a/tests/gtest/plugin_ofx_misc_test.cpp b/tests/gtest/plugin_ofx_misc_test.cpp index 42b1946e1..4a7777621 100644 --- a/tests/gtest/plugin_ofx_misc_test.cpp +++ b/tests/gtest/plugin_ofx_misc_test.cpp @@ -426,12 +426,18 @@ TEST(PluginMisc, MergeOver) GTEST_SKIP() << "OFX integration test not enabled"; } - // FIXME: Merge plugin crashes during render. Needs investigation. - // The crash happens inside PluginRenderer::RenderPlugin, likely due to: - // 1. Multi-input plugin handling issues - // 2. Missing parameter initialization for the merge operation - // 3. Clip format compatibility issues between A and B inputs - GTEST_SKIP() << "Merge plugin crashes - needs fix in multi-input handling"; + VideoParams params(320, 240, core::PixelFormat::U8, 4); + TexturePtr input = CreateSolidTexture(params, 0x80); + ASSERT_NE(input, nullptr); + + NodeValueRow row; + row.insert(QString::fromStdString(kOfxImageEffectSimpleSourceClipName), + NodeValue(NodeValue::kTexture, input)); + row.insert(QStringLiteral("Bg"), + NodeValue(NodeValue::kTexture, input)); + + bool result = RenderPlugin("net.sf.openfx.MergePlugin", params, row, true); + EXPECT_TRUE(result) << "Merge plugin should produce output"; } // ============================================================================ @@ -444,12 +450,16 @@ TEST(PluginMisc, Keyer) GTEST_SKIP() << "OFX integration test not enabled"; } - // FIXME: Keyer plugin crashes during render. Needs investigation. - // The crash happens inside PluginRenderer::RenderPlugin. Possible causes: - // 1. Keyer requires additional optional inputs (Bg, InM, OutM) to be explicitly unset - // 2. Parameter initialization issues (keyColor, mode, etc.) - // 3. The plugin explicitly disables UByte support, U16/F32 may need special handling - GTEST_SKIP() << "Keyer plugin crashes - needs investigation"; + VideoParams params(320, 240, core::PixelFormat::U16, 4); + TexturePtr input = CreateSolidTexture(params, 0x80); + ASSERT_NE(input, nullptr); + + NodeValueRow row; + row.insert(QString::fromStdString(kOfxImageEffectSimpleSourceClipName), + NodeValue(NodeValue::kTexture, input)); + + bool result = RenderPlugin("net.sf.openfx.KeyerPlugin", params, row, true); + EXPECT_TRUE(result) << "Keyer plugin should produce output"; } // ============================================================================