Conversation
… ends with other postfix
…ths from environment variable
…d widget '2dsize'
- handle the Microsoft Visual C++ compiler in module CMakeLists.txt - Change every copyright to the year 2025 - Use C99 variables in the for loop initializer declare style - Clang format and include <stdbool.h> explicitly - specify which versions of OpenFX supported - mention OpenFX header files URL in mlt_openfx.c - using mlt_image and its functions in src/modules/openfx/filter_openfx.c
…t frame to the suitable format
…escribe in context action required by some plugins such as net.sf.openfx.Mirror to function
| if (values == NULL) \ | ||
| return kOfxStatErrMemory; \ | ||
| values[index] = value; \ | ||
| /* TODO: the values array is never freed. */ \ |
There was a problem hiding this comment.
This memory leak needs to be fixed before we can merge.
| char **v = mlt_properties_get_data(p, "v", &length); | ||
| if (length == 0 || length <= index) { | ||
| char **values | ||
| = realloc(v, sizeof(char *) * (index + 1)); // TODO: where is this allocation freed? |
There was a problem hiding this comment.
This memory leak needs to be fixed before we can merge.
| if (values == NULL) | ||
| return kOfxStatErrMemory; | ||
| values[index] = strdup( | ||
| value); // TODO: free old value if exists, and free values on property reset |
There was a problem hiding this comment.
This memory leak needs to be fixed before we can merge.
| mlt_properties_set_data(p, "v", values, index + 1, NULL, NULL); | ||
| } else { | ||
| // TODO: Shouldn't value be duplicated here? The caller might free it after setting the property, and the old value should be freed if exists. | ||
| v[index] = value; // TODO: Free the old value if exists, and free values on property reset |
There was a problem hiding this comment.
This memory leak needs to be fixed before we can merge.
| vt *values = realloc(v, sizeof(vt) * (count)); \ | ||
| if (values == NULL) \ | ||
| return kOfxStatErrUnknown; \ | ||
| memcpy(values, value, sizeof(vt) * (count)); \ |
There was a problem hiding this comment.
How does this work for type string? Are the string pointers being copied instead of copying the whole string?
|
I still can not get the mirror filter to work: Using a single parameter like this has no effect: Using two parameters crashes every time: |
There was a problem hiding this comment.
Pull request overview
This PR adds OpenFX 1.5 plugin support to MLT, continuing work from PR #1186. It provides a module for processing videos using OpenFX plugins, filtering out unsupported plugins (draw suite, OpenGL) and handling plugin identifiers containing colons by replacing them with ^.
Changes:
- Adds complete OpenFX module infrastructure including header files from the OpenFX specification
- Implements MLT integration layer for OpenFX plugins with host suite implementations
- Adds CMake build configuration with glib dependency and GPL licensing
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| CMakeLists.txt | Adds MOD_OPENFX option and glib dependency check |
| CMakePresets.json | Sets MOD_OPENFX to OFF by default |
| src/modules/CMakeLists.txt | Adds openfx subdirectory to build |
| src/modules/openfx/CMakeLists.txt | Defines mltopenfx module build configuration |
| src/modules/openfx/factory.c | Plugin discovery and registration from OFX_PLUGIN_PATH |
| src/modules/openfx/filter_openfx.c | Main filter implementation for OpenFX effects |
| src/modules/openfx/mlt_openfx.h | Common function declarations for OpenFX integration |
| src/modules/openfx/filter_openfx.yml | Metadata for the OpenFX filter |
| src/modules/openfx/openfx/LICENSE.md | BSD-3-Clause license for OpenFX headers |
| src/modules/openfx/openfx/include/*.h | OpenFX 1.5 API header files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dli[0] = 'g'; | ||
|
|
||
| OfxPlugin *(*GetPluginFn)(int nth) = mlt_properties_get_data(mltofx_dl, dli, NULL); |
There was a problem hiding this comment.
The variable dli is being modified in-place by changing its first character (e.g., line 209 dli[0] = 'g'). This pattern is repeated throughout the code and makes it difficult to understand the intent. This appears to be using string mutation as a poor man's hash key, which is error-prone and hard to maintain.
| dli[0] = 'g'; | |
| OfxPlugin *(*GetPluginFn)(int nth) = mlt_properties_get_data(mltofx_dl, dli, NULL); | |
| /* Construct a derived key based on dli without mutating the original string. */ | |
| char dli_key[strlen(dli) + 1]; | |
| strcpy(dli_key, dli); | |
| dli_key[0] = 'g'; | |
| OfxPlugin *(*GetPluginFn)(int nth) = mlt_properties_get_data(mltofx_dl, dli_key, NULL); |
| char *dli = mlt_properties_get(pb, "dli"); | ||
| int index = mlt_properties_get_int(pb, "index"); | ||
|
|
||
| OfxGetPluginFn get_plugin = mlt_properties_get_data(mltofx_dl, dli, NULL); | ||
| OfxPlugin *pt = get_plugin(index); | ||
|
|
||
| dli[0] = 's'; |
There was a problem hiding this comment.
The variable dli is modified by changing its first character from 'g' to 's'. This is manipulating a string in a way that could be fragile and unclear. Consider using separate variables for different purposes or a more explicit naming scheme.
| if ((bni = strstr(name, ".ofx.bundle")) != NULL && bni[11] == '\0') { | ||
| char *barename = g_strndup(name, (int) (bni - name) + 4); | ||
| size_t name_len = (size_t) (bni - name) + 4 + 7; | ||
| // 12b sizeof `Contents` word, 1 sizeof null byte | ||
| char *binpath = malloc(dir_len + name_len + 12 + (name_len - 7) + archstr_len | ||
| + 1); | ||
| sprintf(binpath, "%s/%s/Contents/%s/%s", dir, name, OFX_ARCHSTR, barename); | ||
|
|
||
| void *dlhandle = dlopen(binpath, RTLD_LOCAL | RTLD_LAZY); | ||
|
|
||
| ofx_set_host = dlsym(dlhandle, "OfxSetHost"); | ||
|
|
||
| ofx_get_plugin = dlsym(dlhandle, "OfxGetPlugin"); | ||
| ofx_get_number_of_plugins = dlsym(dlhandle, "OfxGetNumberOfPlugins"); | ||
| if (!ofx_get_plugin || !ofx_get_number_of_plugins) | ||
| goto parse_error; | ||
| NumberOfPlugins = ofx_get_number_of_plugins(); | ||
|
|
||
| char dl_n[16] = { | ||
| '\0', | ||
| }; | ||
| sprintf(dl_n, "%d", dli); | ||
|
|
||
| mlt_properties_set_data(mltofx_dl, dl_n, dlhandle, 0, NULL, NULL); | ||
| dl_n[0] = '\0'; | ||
| sprintf(dl_n, "gn%d", dli); | ||
| mlt_properties_set_data(mltofx_dl, | ||
| dl_n, | ||
| ofx_get_number_of_plugins, | ||
| 0, | ||
| (mlt_destructor) mlt_properties_close, | ||
| NULL); | ||
| dl_n[0] = '\0'; | ||
| sprintf(dl_n, "s%d", dli); | ||
| mlt_properties_set_data(mltofx_dl, | ||
| dl_n, | ||
| ofx_set_host, | ||
| 0, | ||
| (mlt_destructor) mlt_properties_close, | ||
| NULL); | ||
|
|
||
| dl_n[0] = '\0'; | ||
| sprintf(dl_n, "g%d", dli); | ||
| mlt_properties_set_data(mltofx_dl, | ||
| dl_n, | ||
| ofx_get_plugin, | ||
| 0, | ||
| (mlt_destructor) mlt_properties_close, | ||
| NULL); | ||
|
|
||
| dli++; | ||
|
|
||
| if (ofx_get_plugin == NULL) | ||
| goto parse_error; | ||
|
|
||
| for (int i = 0; i < NumberOfPlugins; ++i) { | ||
| OfxPlugin *plugin_ptr = ofx_get_plugin(i); | ||
| if (!plugin_ptr) | ||
| goto parse_error; | ||
|
|
||
| int detected = mltofx_detect_plugin(plugin_ptr); | ||
|
|
||
| if (!detected) | ||
| continue; | ||
|
|
||
| char *s = NULL; | ||
| size_t pluginIdentifier_len = strlen(plugin_ptr->pluginIdentifier); | ||
| s = malloc(pluginIdentifier_len + 8); | ||
| sprintf(s, "openfx.%s", plugin_ptr->pluginIdentifier); | ||
|
|
||
| // if colon `:` exists in plugin identifier | ||
| // change it to accent sign `^` because `:` | ||
| // can cause issues with mlt if put in filter | ||
| // name | ||
| char *str_ptr = strchr(s, ':'); | ||
| while (str_ptr != NULL) { | ||
| *str_ptr++ = '^'; | ||
| str_ptr = strchr(str_ptr, ':'); | ||
| } | ||
|
|
||
| mlt_properties p; | ||
| p = mlt_properties_new(); | ||
| mlt_properties_set_data(mltofx_context, | ||
| s, | ||
| p, | ||
| 0, | ||
| (mlt_destructor) mlt_properties_close, | ||
| NULL); | ||
|
|
||
| mlt_properties_set(p, "dli", dl_n); | ||
| mlt_properties_set_int(p, "index", i); | ||
|
|
||
| MLT_REGISTER(mlt_service_filter_type, s, filter_openfx_init); | ||
| MLT_REGISTER_METADATA(mlt_service_filter_type, | ||
| s, | ||
| metadata, | ||
| "filter_openfx.yml"); | ||
| free(s); | ||
| } | ||
|
|
||
| parse_error: |
There was a problem hiding this comment.
Memory leak: barename and binpath are allocated but not freed when jumping to parse_error before they are freed at the bottom of the loop. The goto parse_error statements at lines 212, 250, and 255 will leak these allocations.
Work on openfx module continued from this PR: #1186