Conversation
Testing with the size parameter for CropOFX
There was a problem hiding this comment.
Pull request overview
This pull request updates the OpenFX module to represent OFX 2D/3D parameter types using MLT’s rect property type (and adjusts core property semantics/tests accordingly), aiming to simplify parameter handling and metadata generation.
Changes:
- Treat
mlt_prop_rectas distinct from opaquemlt_prop_data(including updating serialization andget_data()behavior) and add a unit test to lock this in. - Extend OpenFX parameter mapping to cover
Double3D/Integer3Dand representDouble2D/Integer2Dasrectin generated metadata/defaults. - Add logic to convert normalized
Double2Ddefaults to canonical coordinates using project extent when reading defaults.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/test_properties/test_properties.cpp | Adds a regression test asserting rect properties are not exposed via get_data(). |
| src/framework/mlt_property.c | Adjusts rect/data type behavior for destructor handling, string serialization, and get_data() semantics. |
| src/modules/openfx/mlt_openfx.h | Adds new OpenFX setter types for 3D parameters. |
| src/modules/openfx/mlt_openfx.c | Maps OFX 2D/3D params to rect, sets rect defaults, and adds normalized-default handling. |
| src/modules/openfx/filter_openfx.c | Updates runtime param propagation to select correct OFX setter based on native param type for rect. |
| // Declare support for normalised default coordinate systems on Double2D/Double params. | ||
| // When present, plugins call setDefaultCoordinateSystem(eCoordinatesNormalised) on param | ||
| // descriptors whose defaults are expressed in normalized [0,1] coordinates, which our | ||
| // metadata loop then reads as normalized_coordinates="yes". | ||
| propSetString(host_properties, | ||
| kOfxParamPropDefaultCoordinateSystem, | ||
| 0, | ||
| kOfxParamCoordinatesNormalised); |
There was a problem hiding this comment.
This probably needs more investigation and testing. But the suggestion is probably good. I was using AI to try to find workarounds for the default normalization problem and this is one suggestion it came up with after inspecting Natron and plugin source code. But it is probably unnecessary.
Note this Natron documentation which explicitly lists the Crop plugin as not working with Davinci.
https://github.com/NatronGitHub/openfx-misc?utm_source=chatgpt.com#the-default-parameters-are-too-small-on-davinci-resolve
There is a bug in mlt_property that interprets a rect structure as data and tries to interpret it as nested properties when traversing the metadata properties. This works around that.
Map all 2D and 3D types to MLT rect