Fix avdeinterlace buffering

[alcinos]

Problem

avdeinterlace detects discontinuities using timeline positions only. Consecutive playlist clips can reuse the same source while restarting its position, without interrupting timeline continuity. YADIF (and likely other interlacer using look-ahead) then returns stale buffered look-ahead content at the cut.

Fix

Track the expected original source position alongside the timeline position. Reset the deinterlacer when either becomes discontinuous.

[bmatherly]

This should not be necessary if you are using the avdeinterlace link as intended. The link should be added automatically as a normalizer link, not manually by the application.

If you use the loader constructor for MLT:Chain, then it will call mlt_chain_attach_normalizers() and the avdeinterlace filter will only be applied if necessary.

mlt/src/mlt++/MltChain.cpp

Line 41 in 77ae5f8

mlt_chain_attach_normalizers(instance);

[bmatherly]

This test is still doing unexpected things. The source should belong to the chain and it should not have filters attached to it. You are putting filters before the normalizers. Attach the filters to the chain, not to the source producer.

Can you reproduce your problem without adding filters to the encapsulated source producer?

[alcinos]

Pushed a cleaner test where blipflash is rendered interlaced first, then used in the test instead of the dummy clips using filters approach I did earlier. It still reproduces the problem.

[alcinos]

Thanks for the reply

I may be missing something, but I do not see how the way the avdeinterlace link is added matters for this bug.
The issue is that cuts on one chain share its normalizer link, and currently rewinding a source on a continuous playlist isn't counted as a discontinuity, which leads to stale buffer image being pulled.

I updated the test to use the Chain constructor. The test still fails on master, passes after the fix. Let me know what you think.

Separately, one CI is red. I believe this is due to using old Ffmpeg (4.4). The reason seems to be as follows:

1. Filter-graph setup in link_avdeinterlace.c, link_avfilter.c and filter_avfilter.c pass colorspace and range to FFmpeg's buffersrc via av_opt_set_int, and abort setup (goto fail) if that fails.
2. Those buffersrc options don't exist in FFmpeg 4.4 so setup always fails with "Cannot set src colorspace".
3. In link_avdeinterlace.c, when setup fails, fdata stays NULL. link_get_image logs "No Filtergraph" and leaves its loop, then runs av_frame_unref(fdata->avoutframe) unconditionally. The pointer being NULL, this segfaults.

Guidance appreciated on how to fix that. My 2 cents is either to drop support for 4.4 or add a version guard on the buffersrc, which will make the segfault go away at the cost of differences in behavior depending on ffmpeg version.

[bmatherly]

Thank you for changing the test. I do not want a test in our repository that sets a bad example for how the framework should be used.

The issue is that cuts on one chain share its normalizer link, and currently rewinding a source on a continuous playlist isn't counted as a discontinuity, which leads to stale buffer image being pulled.

That is a helpful comment. Now I understand the problem better.

In general, I do not think many applications are sharing a single producer multiple times in a playlist. The framework does not prohibit it. But I do not know if it is a best practice. I'll let @ddennedy chime in on that.

Using mlt_frame_original_position() in a link as a continuity indicator is not a good idea because there would be a link ahead of it that is skipping/repeating frames. But, in the specific case of avdelinterlace, that is unlikely because it should usually be the first normalizer. But again, I don't really want to set a bad example in the code.

Can you work around this limitation by not sharing producers?

FFmpeg 4.4 so setup always fails with "Cannot set src colorspace".

@ddennedy how would you feel about dropping support for FFMpeg 4.4? It looks like it is not actually working anyway (even though it compiles)

[alcinos]

I do not think many applications are sharing a single producer multiple times in a playlist

I cannot speak for all applications, but as a kdenlive maintainer I can tell you that kdenlive keeps one producer per bin clip per track, and every timeline clip is a cut of it. We got hit by the buffering bug, hence this PR to attempt to resolve it.

I do not know if it is a best practice

I do not know either, but just pointing out that MLT's own function mlt_playlist_split() makes the second half with a cut of the same parent. So that would lead me to believe "sharing a single producer multiple times in a playlist" is not so much of an anti-pattern.

Using mlt_frame_original_position() in a link as a continuity indicator is not a good idea because there would be a link ahead of it that is skipping/repeating frames

Is there an alternative preferred continuity signal? For example, could the link detect that the frame came from a different cut?

Can you work around this limitation by not sharing producers?

Currently in kdenlive it's a pretty deeply ingrained pattern, so it would not be a "workaround" so much as a redesign. It would be great if we can avoid it.

[ddennedy]

Yes, it is perfectly fine to share producers like this. I would only caution against using the same shared producer in a transition. Yes, we can drop FFmpeg 4. Debian stable has 7. This is due to using ubuntu-22.04 in .github/workflows/build-linux.yml

[bmatherly]

Based on the discussion I think that using the original frame position is our best option.

Could this simpler patch solve the problem?

diff --git a/src/modules/avformat/link_avdeinterlace.c b/src/modules/avformat/link_avdeinterlace.c
index c945603a..d482dc8b 100644
--- a/src/modules/avformat/link_avdeinterlace.c
+++ b/src/modules/avformat/link_avdeinterlace.c
@@ -397,11 +397,11 @@ static int link_get_image(mlt_frame frame,
 
     mlt_service_lock(MLT_LINK_SERVICE(self));
 
-    if (pdata->method != method || pdata->expected_frame != mlt_frame_get_position(frame)) {
+    if (pdata->method != method || pdata->expected_frame != mlt_frame_original_position(frame)) {
         mlt_log_debug(MLT_LINK_SERVICE(self), "Reset: %s\n", mlt_deinterlacer_name(method));
         pdata->reset = 1;
         pdata->continuity_frame = mlt_frame_get_position(frame);
-        pdata->expected_frame = mlt_frame_get_position(frame);
+        pdata->expected_frame = mlt_frame_original_position(frame);
         pdata->method = method;
     }

[alcinos]

@bmatherly I do not think your patch is valid. We need to detect two kinds of discontinuities:

1. the source is discontinuous but the timeline is continuous (eg we put two cuts of the same clip one after another and they do not point to continuous frames).
2. the source is continuous, but the timeline is not (eg cut the clip in half and add some frames in the middle).

Previously, 2 was tackled but not 1. With your patch, it's the opposite. Mine should cover both.
I added a "gap in the middle" test, which fails with your patch. Let me know what you think.

For ffmpeg 4 support drop, what's the plan, should I address it in this PR as well?

[bmatherly]

Previously, 2 was tackled but not 1.

I think number two is fine with this patch. If the source is cut and frames are inserted in between, then the avdeinterlace link would still see continuous frames because the "in between" frames would never reach it.

In fact, I think we have the opposite problem right now - using the playlist frame continuity can cause the link to reset when it is not necessary.

Can you test it?

[alcinos]

I see what you mean. I pushed a corrected version of your idea, lmk what you think

[bmatherly]

I see what you mean. I pushed a corrected version of your idea, lmk what you think

I think we have landed on a good solution. Thank you for your collaboration.

I have a PR here that should fix the build error. Maybe you can rebase on it. Or just cherry pick it into this PR.
#1342

[Copilot]

🟡 Changes recommended

The implementation drops timeline-discontinuity detection instead of tracking both timeline and source continuity as described.

1 open finding

• Track source continuity separately from timeline continuity · New

What changed in this PR

Updates avdeinterlace buffering to detect source-position discontinuities and prevent stale look-ahead frames.

Changes:

• Tracks continuity using original source positions.
• Adds regression coverage for repeated source cuts.
• Guards AVFrame cleanup when filter data is unavailable.

File	Description
src/​modules/​avformat/​link_avdeinterlace.c	Revises buffering continuity and cleanup logic.
src/​tests/​test_mod_avformat/​test_mod_avformat.cpp	Adds source-discontinuity regression testing.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[bmatherly]

This is good to go from my perspective.