WIP: DynamicEffects: Add code for Linux with tests #345
Conversation
|
@kirillmakhonin Wow! This looks really interesting, I'm excited to check it out. I love the idea of making libopenshot more modular, period, and breaking out the effects seems like one of the more natural ways to do that. I can even imagine porting all of our built-in effects to build as runtime-loaded modules, perhaps, and doing away with the "built-in vs. external" divide entirely. But for now, this sounds like a perfect first step. I'll give it a spin and post more feedback once I've had a chance to digest it more fully, and then hopefully a review some time in the next couple of days. Some thoughts on your improvements list, for starters:
Eh... I mean, maybe? If there are effects that might benefit from sharing code, I can see there being an advantage to that. (But it's not like any of our EXISTING effects share even a single line of code, as things stand today.) Unless there's some practical advantage to building multi-effect libraries, though, my knee-jerk inclination is to keep each one in its own module. Linux's libdl, at least, is pretty efficient about loading multiple objects. But maybe I'll reconsider that or be talked out of it later, currently it's just a snap judgement.
Yeah, versioning will definitely be important here — potentially of both the source (versioned for API changes) and the objects. (libopenshot should probably require that the objects export some build-ident signature that matches its own copy, so it only loads modules if they were compiled for that exact
Best thing might be for us to define a Settings variable that holds the plugin path, that'll provide callers with an already-established API for configuring it. And supporting an envvar for runtime overrides (or additions?) makes sense as well. Some other possible items to consider:
Sorry for the brain-dump, I'll probably have more focused feedback once I've tried this out and gotten more of a hands-on feel for it. But thanks again for the contribution, this really seems like it'll be an awesome evolutionary next-step for libopenshot! |
|
@ferdnyc, I did some updates, could you please review them? |
tests/CMakeLists.txt
Outdated
| ELSE() | ||
| # Link libraries to the new executable | ||
| target_link_libraries(openshot-test openshot ${UNITTEST++_LIBRARY}) | ||
| ENDIF(NOT DISABLE_EXTERNAL_EFFECTS) |
There was a problem hiding this comment.
Couple issues here:
-
DynamicEffects_Tests.cppwill be in the test target sources list even whenDISABLE_EXTERNAL_EFFECTSis set. That's okaaaay technically, because the whole file's wrapped in#ifdef TEST_PLUGIN, but if nothing else it's an extra source file being compiled for no reason. -
Bigger issue, if you want to test the
dlopen()functionality ofEffectInfo::LoadEffect, I suspect you can't link the executable with the library you're trying todlopen()or it will invalidate the test.Because
effect-superbluris added to thetarget_link_libraries()ofopenshot-test, it'll be dynamically linked with the executable:$ cd build $ make openshot-test $ ldd tests/openshot-test |grep -i effect libeffect-superblur.so => /home/ferd/rpmbuild/REPOS/libopenshot/build/external-effects/super-blur/libeffect-superblur.so (0x00007f41707c6000)
The dynamic loader is sure to be smart enough that, when you call
dlopen()on that file path, it's not going to load anything at all — it's just going to return a handle to the library that was already loaded at the start of program execution.To test dynamic-loading from the library file on disk, you don't want to link the executable to it. It should have no knowledge of that library prior to
dlopen()being given the pathname to load it from.
I guess my preference would be something more like this:
| ENDIF(NOT DISABLE_EXTERNAL_EFFECTS) | |
| # Link libraries to the new executable | |
| target_link_libraries(openshot-test openshot ${UNITTEST++_LIBRARY}) | |
| if (NOT DISABLE_EXTERNAL_EFFECTS) | |
| target_sources(openshot-test PRIVATE DynamicEffects_Tests.cpp) | |
| target_compile_definitions(openshot-test PRIVATE | |
| TEST_PLUGIN="${PROJECT_BINARY_DIR}/external-effects/super-blur/libeffect-superblur.so" | |
| TEST_PLUGIN_NAME="SuperBlur") | |
| endif () |
That'll add in the source file and the definitions iff enabled, and leave the library unlinked so it'll have to be loaded from the path in the TEST_PLUGIN definition.
There was a problem hiding this comment.
I made compilation of file DynamicEffects_Tests.cpp optional, also I remove static linking with effect library.
@ferdnyc could you, please, have a look?
There was a problem hiding this comment.
Wow, you are OLD-school CMake, huh? 😆 Well, that's cool, whatever works — and this does, thanks. One day I'll drag this project kicking and screaming into the Modern CMake world, but that day is not today.
I'm actually a little surprised add_definitions() is still effective even when it comes after the creation of the executable target (as opposed to target_compile_definitions() operating on the target), but it appears that's the case.
I made compilation of file
DynamicEffects_Tests.cppoptional,
And the add_dependency() is a good idea, I missed that.
also I remove static linking with effect library.
*dynamic linking. (A bit pedantic, I admit, but the distinction is critical. Static linking compiles the library right into the executable and creates no external dependencies. With a shared library the runtime dependency is still dynamic linking — as opposed to dynamic loading via dlopen(), which is what you're doing instead, and requires no prior linkage between the executable and the shared object file.)
| #if defined(__linux__) | ||
| EffectBase* EffectInfo::LoadEffect(std::string location){ | ||
| pthread_mutex_lock(&m_mutex); | ||
|
|
||
| void * file = dlopen(location.c_str(), RTLD_NOW); | ||
| if (file == nullptr){ | ||
| pthread_mutex_unlock(&m_mutex); | ||
| throw InvalidFile("Can not open file", location.c_str()); | ||
| } | ||
| void * factory_ptr = dlsym(file, "factory"); | ||
|
|
||
| if (factory_ptr == nullptr){ | ||
| pthread_mutex_unlock(&m_mutex); | ||
| dlclose(file); | ||
| throw InvalidFile("Can not find requested plugin API in file", location.c_str()); | ||
| } | ||
|
|
||
| EffectBase * (*factory)(uint16_t); | ||
| factory = (EffectBase* (*)(uint16_t)) factory_ptr; | ||
|
|
||
| EffectBase* instance = factory(OPENSHOT_PLUGIN_API_VERSION); | ||
|
|
||
| if (instance == nullptr){ | ||
| pthread_mutex_unlock(&m_mutex); | ||
| dlclose(file); | ||
| throw InvalidFile("Plugin does not support current version of openshot", location.c_str()); | ||
| } | ||
|
|
||
| m_loadedDynamicEffects.insert(make_pair(std::string(instance->info.name), factory)); | ||
| m_loadedDynamicHandles.insert(m_loadedDynamicHandles.end(), file); | ||
|
|
||
| pthread_mutex_unlock(&m_mutex); | ||
| return instance; | ||
| } | ||
|
|
||
| void EffectInfo::UnloadDynamicEffects(){ | ||
| pthread_mutex_lock(&m_mutex); | ||
|
|
||
| for (auto & handle : m_loadedDynamicHandles){ | ||
| dlclose(handle); | ||
| } | ||
|
|
||
| m_loadedDynamicEffects.clear(); | ||
| m_loadedDynamicHandles.clear(); | ||
| pthread_mutex_unlock(&m_mutex); | ||
| } | ||
| #else | ||
| EffectBase* EffectInfo::LoadEffect(std::string location){ | ||
| return NULL; | ||
| } | ||
|
|
||
| void EffectInfo::UnloadDynamicEffects(){ | ||
|
|
||
| } | ||
| #endif | ||
|
|
There was a problem hiding this comment.
This might be cleaner/easier to write as...
EffectBase* EffectInfo::LoadEffect(std::string location){
#if !defined(__linux__)
return NULL;
#endif
pthread_mutex_lock(&m_mutex);
...
}
void EffectInfo::UnloadDynamicEffects(){
#if defined(__linux__)
pthread_mutex_lock(&m_mutex);
for (auto & handle : m_loadedDynamicHandles){
dlclose(handle);
}
m_loadedDynamicEffects.clear();
m_loadedDynamicHandles.clear();
pthread_mutex_unlock(&m_mutex);
#endif
}Primary advantage: If the signature of EffectInfo::LoadEffect() or EffectInfo::UnloadDynamicEffects() changes, we don't have to worry about updating it in two places and making sure they're kept in sync.
There was a problem hiding this comment.
(Sorry, accidentally had an earlier version of this comment included in the review, along with the final one. I've deleted the redundant comment, but the email notification version may have been a bit confusing.)
include/EffectBase.h
Outdated
| @@ -31,6 +31,8 @@ | |||
| #ifndef OPENSHOT_EFFECT_BASE_H | |||
| #define OPENSHOT_EFFECT_BASE_H | |||
|
|
|||
| #define OPENSHOT_PLUGIN_API_VERSION 0x0001 | |||
There was a problem hiding this comment.
Yeah... as you mentioned in the main PR comments, I'd just use the SO version instead. (It's #defined as OPENSHOT_VERSION_SO in OpenShotVersion.h.)
The main reason being, we really don't have an effects API currently (and that's on us). The abstractions for effects use aren't really great. Callers generally need to have knowledge of the API for the specific effects they use, and to an extent that will also include the dynamic ones. (The JSON interfaces do somewhat alleviate that, but more on that later.)
As a result, the plugin compatibility will most likely be tied to the library API even if the plugin-loading API stays the same, and a plugin API versioning scheme just adds another numbering to keep track of.
But if this IS retained, it should be placed in include/OpenShotVersion.h.in instead of here. (Which is the input for a CMake-generated header file that you can include via #include "OpenShotVersion.h", to access any of the versioning macros.)
| #ifdef TEST_PLUGIN | ||
| #if defined(__linux__) |
There was a problem hiding this comment.
I was going to suggest combining these as
#if defined(__linux__) && defined(TEST_PLUGIN)But since in theory we might have plugin-loading implementations for other OSes, leaving them separate may be better. The only thing I'd request is commenting the #endifs as:
#endif // defined(__linux__)
#endif // TEST_PLUGINsince they're way too far away to match up visually.
| } | ||
|
|
||
| #endif | ||
| #endif |
There was a problem hiding this comment.
Minor nit: File doesn't end in a newline.
|
Surprisingly enough, it may be possible to use the same code even on Windows: https://github.com/dlfcn-win32/dlfcn-win32 ( |
|
@kirillmakhonin This looks very promising! Thanks again for your help implementing it. I'm feeling like we should keep iterating on this idea (in this PR), and before we merge, I would suggest we get this working on all 3 OSes. If I need to install additional things on our Windows build server, please let me know. We can also move this into an official branch, so our build servers can play with it and create test builds. Also, most effect metadata comes from Another thought... how would we handle translations for externally loaded effects. I do love the idea of dropping an effect library into a folder, and launching OpenShot, and when detected, those effect libraries would be loaded, translations loaded, icons loaded, etc... and would be completely seamless to the user. 👍 |
Agreed. It's really promising, so it’s worth taking the time to make it a first-class feature that works well for all platforms AND for all consumers. (@jeffski and/or @DylanC may have some thoughts on this, perhaps?)
*nod* And that's a help, but still feels incomplete to me. The JSON interface to properties works for OpenShot, but it's less convenient for direct One of the things I was going to suggest as far as this PR goes, for the unit testing, was to reimplement the Blur tests from libopenshot/tests/Timeline_Tests.cpp Lines 416 to 445 in 76725d9 If you try to actually do that, you immediately run into the snag that, because dynamic effects use the factory pattern to instantiate the effect objects, you lose access to the effect's custom constructors. This isn't how you'd create a SuperBlur object: Keyframe horizontal_radius(5.0);
Keyframe vertical_radius(5.0);
Keyframe sigma(3.0);
Keyframe iterations(3.0);
Blur blur(horizontal_radius, vertical_radius, sigma, iterations);Now, sure, you could use JSON to serialize and apply the appropriate properties, and that works fine for OpenShot, but for libopenshot consumers I think that probably would feel overly complicated and tedious. What might be better is if we come up with a standard setter/getter interface for effect parameters, and we implement that interface in the That'd also make the bindings for effects far less complex. To the point where it might actually be possible to completely drop the SWIG-generated interface for each effect in favor of
Mmm, I hadn't even thought about translations. ...How do we handle translations for built-in effects? We don't, in libopenshot. It's not translated at all. Because of the JSON interface, all parameters already have string names, and those names are used by OpenShot to assemble the translations. OpenShot queries the list of effects in the Effects model using # Append on properties from libopenshot
objects = [openshot.Clip(), openshot.Bars(), openshot.Blur(), openshot.Brightness(),
openshot.ChromaKey(), openshot.ColorShift(), openshot.Crop(), openshot.Deinterlace(), openshot.Hue(), openshot.Mask(),
openshot.Negate(), openshot.Pixelate(), openshot.Saturation(), openshot.Shift(), openshot.Wave()]
# Loop through each libopenshot object
for object in objects:
props = json.loads(object.PropertiesJSON(1))
# Loop through props
for key in props.keys():
object = props[key]
if "name" in object.keys():
effects_text[object["name"]] = "libopenshot (Clip Properties)"
if "choices" in object.keys():
for choice in object["choices"]:
effects_text[choice["name"]] = "libopenshot (Clip Properties)"
# Append Effect Meta Data
e = openshot.EffectInfo()
props = json.loads(e.Json())
# Loop through props
for effect in props:
if "name" in effect:
effects_text[effect["name"]] = "libopenshot (Effect Metadata)"
if "description" in effect:
effects_text[effect["description"]] = "libopenshot (Effect Metadata)"In one sense you could say that this means nothing's changed, so there's no need to worry about it... in libopenshot. But in another sense, it really complicates things. Translations being managed completely external to Which does mean that, if we were to have "built-in" dynamic effects (ones bundled with libopenshot, that OpenShot therefore does have prior knowledge of), translations should still work the same as always. This PR already updates For "third-party" effects, things become significantly trickier. I have no doubt that Qt's QTranslator system is modular enough to handle this situation, but we have to figure out the best way to make use of it. If we're lucky, most of the names of parameters and things should be the same across all effects, so we may be able to reuse the strings already defined. To facilitate that, we may want to define a common translation context for all of the property names, possibly associated with either Even still there are still at least some text strings — the name of the effect itself, its description, etc. — that won't fall into that shared-reuse category. Those would need to be translated individually for each effect. (Users do tend to be somewhat more accepting of strings like that being un-translated, though, if it comes down to it. As long as the actual user interface can be understood.) There's a possibility we could just look for a Or, we could have each plugin incorporate its translation data as a binary blob, and use the Or... something else? |
|
(I resolved the merge conflicts in |
Actually, scratch that. Because of the way they're collected now, all properties already have a common translation context. From #: /home/jonathan/apps/openshot-qt-git/src/windows/views/timeline_webview.py:2408
#: libopenshot (Clip Properties)
msgid "Brightness"
msgstr ""
#: /home/jonathan/apps/openshot-qt-git/src/windows/views/timeline_webview.py:2410
#: libopenshot (Clip Properties)
msgid "Contrast"
msgstr ""So, dynamically-loaded effects would automatically be handled using that same context, and all of the translated strings from existing properties would be available for use with the dynamic effects' properties as well. |
|
Actually, an even better example is: #: libopenshot (Effect Metadata)
msgid "Brightness & Contrast"
msgstr ""
#: libopenshot (Clip Properties)
msgid "Sigma"
msgstr ""
#: libopenshot (Clip Properties)
msgid "Duration"
msgstr ""
#: libopenshot (Effect Metadata)
msgid "Color Saturation"
msgstr ""No source-code contexts at all. (But it seems like strings will be reused for other contexts even if they are defined with a source context. For instance, the |
|
Note to self, regarding translations generally: https://pypi.org/project/distutils_ui/ (Helpers that can be used in a |
|
@ferdnyc I love the idea of bundling a .qm file alongside an effect library. Although, it would also be great if that could be included in the library (not sure how though), like a resource file compiled into the library... hmmm. Like you said, the shared context of OpenShot would still be used to translate strings (if the words have already been translated), but maybe an optional *.qm file would be loaded (if found), and that could be used to fill in the remaining translation gaps in an external effect. If we really implement this correctly, this could end up being a huge deal for OpenShot... and other open-source projects which use libopenshot. The ability to quickly and easily create an effect seems sooo important. We could also make an |
jonoomph
changed the title
|
I've seen this uses |
|
Since |
|
Merge conflicts have been detected on this PR, please resolve. |
This PR adds possibility to develop and integrate effects for libopenshot as a shared library.
Right now it works only for Linux, on other platform this method does not do anything (returns NULL).
As an example of "plugin" I've copied Blur effect with name SuperBlur.
From user-perspective, this feature is needed to have an ability create granular "plugins" w/o entire libopenshot compilation.
Later, this system needs to be improved to: