[eseifert]

The encoding thread loops while the running property is set, but the property was only set after creating the thread. If the thread got to its loop first, it skipped encoding and marked the consumer stopped, and the start function then set running again, so the consumer looked like it was running forever and melt never exited, with an empty output file. A thread failing early had the same effect. Set the property before creating the thread.

Reproduced by making the race deterministic: an LD_PRELOAD wrapper that sleeps 2 s in the main thread after each pthread_create() (simulating the main thread being descheduled, as on a loaded CI runner). melt color:red out=24 -consumer avformat:out.mp4 vcodec=libx264 an=1 then hung with a 0 byte output on master in 3 of 3 runs (killed after 30 s), the same state as in #1333 with no consumer thread left, main thread polling consumer_is_stopped. With the fix all 3 runs finished with the normal output. Normal encodes are unchanged.

Fixes #1333

[Copilot]

🟢 Approval recommended

The localized ordering change resolves the identified startup race without altering the existing stop or cleanup paths.

No open issues 🎉

What changed in this PR

This PR addresses the avformat consumer startup race associated with #1333, which could leave melt waiting indefinitely with an empty output file.

Changes:

• Sets running before creating the encoding thread so the worker sees the correct initial state and an early exit is not overwritten by startup.

File	Description
src/​modules/​avformat/​consumer_avformat.c	Moves the running-state update ahead of pthread_create().

_{Review effort: Balanced}

[bmatherly]

Upon inspection, this seems fine to me. But maybe it will just surface some other race condition that we can not think of. It would be better if the threads were synchronized with condition variables that also protected the "running" property. I do not know if it is worth the effort/risk to redesign the synchronization.

[eseifert]

Thanks for looking. I agree that condition variables protecting running would be the cleaner design. I kept this PR minimal so it can go in on its own as the fix for #1333. I also checked if it could expose another race, but running is only used in start, stop, is_stopped and the thread loop. All goes through the locked property API. pthread_create() guarantees the new thread sees the value set before it, so the thread can now only see 0 after consumer_stop() or its own mlt_consumer_stopped(). On master, start could overwrite that 0 afterwards. consumer_null, consumer_multi and consumer_xml already set running before creating their thread, so avformat would now match them.

One existing issue that will remain is that the pthread_create() result isn’t checked. So, a failed thread creation would leave running set, as it does on master. If you think the redesign is worth it, I could try it in a separate PR.

[bmatherly]

I did a little more research and found the reason it is like this:
de3106c

This change would effectively undo that fix. We should spend some more time making sure we won't have a regression. Maybe the code has changed enough (in 15 years!) that the previous bug would not happen any longer.

Another idea: synchronize the thread creation so that the function does not exit until the thread is actually running and waiting for input. The thread could set the "running" flag when it is ready.