Conversation
Adds 2 new functions: `mlt_frame_get_image_from_service()` `mlt_frame_get_image_with_fx_cut()`
There was a problem hiding this comment.
🟡 Changes recommended
FX cuts can become ineffective without a transition or when transition tracks are reversed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates fx_cut routing so filters affect only lower tracks while higher tracks composite afterward.
Changes:
- Adds reusable frame image-routing callbacks.
- Moves FX routing into transition processing.
- Adds regression tests, API exports, and documentation.
File summaries
| File | Description |
|---|---|
NEWS |
Documents the corrected behavior and APIs. |
src/framework/mlt.vers |
Exports the new callbacks. |
src/framework/mlt_frame.c |
Implements image sharing and FX routing. |
src/framework/mlt_frame.h |
Declares the new public APIs. |
src/framework/mlt_playlist.h |
Documents track-order behavior. |
src/framework/mlt_tractor.c |
Changes FX handling during track stacking. |
src/framework/mlt_transition.c |
Applies FX filters to lower-track frames. |
src/tests/test_tractor/test_tractor.cpp |
Tests FX track ordering and standalone behavior. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Image ownership leaks and an uncovered higher-track routing case remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/framework/mlt_frame.c:1124
- When an fx filter edits the underlying image in place,
srcanddsthold the same pointer here. Callingmlt_frame_set_image(dst, image, ..., NULL)with that same pointer clearsdst's existing destructor (mlt_property_set_data()does this atsrc/framework/mlt_property.c:308-315), while the fx frame only has a non-owning reference. The final share-back therefore leaks the image buffer on every such frame. Preserve the existing ownership when the pointers are identical.
int size = 0;
uint8_t *image = mlt_properties_get_data(MLT_FRAME_PROPERTIES(src), "image", &size);
mlt_frame_set_image(dst, image, 0, NULL);
copy_image_state(dst, src);
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Image-state sharing can preserve stale optional data or copy a stale image property instead of the returned buffer.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Transition routing can still bypass ordinary lower-index tracks positioned between the selected A frame and an adjustment clip.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟢 Approval recommended
The updated implementation addresses the prior findings and includes broad regression coverage for the affected routing scenarios.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Adds 2 new functions:
mlt_frame_prepend_image_from_service()mlt_frame_push_image_with_fx_cut()