Fix crashes when opening or closing readers and exporting raw video

.github/workflows/ci.yml

@@ -133,6 +133,13 @@ jobs:
           export QT_QPA_PLATFORM=offscreen
           cmake --build build --target coverage -- VERBOSE=1 || true
 
+      - name: Check reader and raw-video stability
+        if: ${{ matrix.compiler.cc == 'gcc' && runner.os == 'linux' }}
+        timeout-minutes: 10
+        run: |
+          sudo apt install -y valgrind
+          bash tests/check-stability.sh build
+
       - name: Install libopenshot
         run: |
           # Stage all installs (including absolute Python paths) under our workspace/install

src/FFmpegReader.cpp

@@ -261,10 +261,10 @@ int FFmpegReader::IsHardwareDecodeSupported(int codecid)
 #endif // USE_HW_ACCEL
 
 void FFmpegReader::Open() {
+	// Check lifecycle state only after any in-flight Open/Close has finished.
+	const std::lock_guard<std::recursive_mutex> lock(getFrameMutex);
 	// Open reader if not already open
 	if (!is_open) {
-		// Prevent async calls to the following code
-		const std::lock_guard<std::recursive_mutex> lock(getFrameMutex);
 
 		// Initialize format context
 		pFormatCtx = NULL;
@@ -749,10 +749,10 @@ void FFmpegReader::Open() {
 }
 
 void FFmpegReader::Close() {
+	// A queued close must not clean up contexts released by an earlier caller.
+	const std::lock_guard<std::recursive_mutex> lock(getFrameMutex);
 	// Close all objects, if reader is 'open'
 	if (is_open) {
-		// Prevent async calls to the following code
-		const std::lock_guard<std::recursive_mutex> lock(getFrameMutex);
 
 		// Mark as "closed"
 		is_open = false;

src/FFmpegWriter.cpp

@@ -2337,38 +2337,34 @@ bool FFmpegWriter::write_video_packet(std::shared_ptr<Frame> frame, AVFrame *fra
 
 	if (oc->oformat->flags & AVFMT_RAWPICTURE) {
 #endif
-		// Raw video case.
-#if IS_FFMPEG_3_2
-		AVPacket* pkt = av_packet_alloc();
-#else
-		AVPacket* pkt;
-		av_init_packet(pkt);
-#endif
-
-	av_packet_from_data(
-			pkt, frame_final->data[0],
-			frame_final->linesize[0] * frame_final->height);
-
-		pkt->flags |= AV_PKT_FLAG_KEY;
-		pkt->stream_index = video_st->index;
-
-		// Set PTS (in frames and scaled to the codec's timebase)
-		pkt->pts = video_timestamp;
-		pkt->duration = av_rescale_q(1, av_make_q(info.fps.den, info.fps.num), video_codec_ctx->time_base);
-
-		/* write the compressed frame in the media file */
-		int error_code = av_interleaved_write_frame(oc, pkt);
+		// The packet owns a separate, padded buffer. write_frame() still owns
+		// frame_final, so handing its data to av_packet_from_data would free it twice.
+		AVPacket packet = {};
+		const PixelFormat format = static_cast<PixelFormat>(frame_final->format);
+		const int size = AV_GET_IMAGE_SIZE(format, frame_final->width, frame_final->height);
+		int error_code = size < 0 ? size : av_new_packet(&packet, size);
+		if (error_code >= 0) {
+			// Copy every plane, not just the first plane's stride * height.
+			error_code = av_image_copy_to_buffer(packet.data, packet.size,
+				frame_final->data, frame_final->linesize, format,
+				frame_final->width, frame_final->height, 1);
+		}
+		if (error_code >= 0) {
+			packet.flags |= AV_PKT_FLAG_KEY;
+			packet.stream_index = video_st->index;
+			packet.pts = packet.dts = video_timestamp;
+			packet.duration = av_rescale_q(1, av_make_q(info.fps.den, info.fps.num), video_codec_ctx->time_base);
+			av_packet_rescale_ts(&packet, video_codec_ctx->time_base, video_st->time_base);
+			error_code = av_interleaved_write_frame(oc, &packet);
+		}
+		AV_FREE_PACKET(&packet);
 		if (error_code < 0) {
 			Logger::Instance()->AppendDebugMethod(
 				"FFmpegWriter::write_video_packet ERROR ["
 					+ av_err2string(error_code) + "]",
 				"error_code", error_code);
 			return false;
 		}
-
-		// Deallocate packet
-		AV_FREE_PACKET(pkt);
-
 	} else
 	{
 

tests/FFmpegReader.cpp

@@ -19,6 +19,8 @@
 #include <cstdlib>
 #include <ctime>
 #include <chrono>
+#include <future>
+#include <thread>
 
 #include "openshot_catch.h"
 
@@ -108,6 +110,60 @@ struct TemporaryFileGuard {
 
 }
 
+TEST_CASE("Queued reader lifecycle calls recheck state after locking",
+          "[libopenshot][ffmpegreader][lifecycle]")
+{
+	class LockedReader : public FFmpegReader {
+	public:
+		using FFmpegReader::FFmpegReader;
+		using ReaderBase::getFrameMutex;
+	};
+	LockedReader reader(std::string(TEST_MEDIA_PATH) + "sintel_trailer-720p.mp4");
+	const bool initially_open = GENERATE(false, true);
+	const bool closing = GENERATE(false, true);
+	CAPTURE(initially_open, closing);
+	if (initially_open) reader.Open();
+
+	// Hold the lifecycle lock while starting the worker. Exercise
+	// Open/Open, Close/Close, and both mixed call orders.
+	std::unique_lock<std::recursive_mutex> lock(reader.getFrameMutex);
+	std::promise<void> started, finished;
+	auto done = finished.get_future();
+	std::exception_ptr error;
+	std::thread pending([&] {
+		started.set_value();
+		try {
+			if (closing) reader.Close(); else reader.Open();
+		} catch (...) { error = std::current_exception(); }
+		finished.set_value();
+	});
+	started.get_future().wait();
+	// Give the worker time to reach the held mutex. This exercises contention,
+	// but cannot guarantee scheduling on every platform.
+	const bool waited = done.wait_for(std::chrono::milliseconds(100)) == std::future_status::timeout;
+	std::exception_ptr foreground_error;
+	try {
+		if (initially_open) reader.Close(); else reader.Open();
+	} catch (...) { foreground_error = std::current_exception(); }
+	AVFormatContext* context = reader.pFormatCtx;
+	lock.unlock();
+	pending.join();
+
+	REQUIRE(waited);
+	REQUIRE(foreground_error == nullptr);
+	REQUIRE(error == nullptr);
+	CHECK(reader.IsOpen() == !closing);
+	// A queued Open must reuse the existing context, not leak it and reopen.
+	if (closing == initially_open) CHECK(reader.pFormatCtx == context);
+	CHECK_NOTHROW(reader.Close());
+	CHECK_NOTHROW(reader.Close());
+	reader.Open();
+	CHECK(reader.GetFrame(1)->GetWidth() > 0);
+	CHECK(reader.GetFrame(30)->GetWidth() > 0);
+	CHECK(reader.GetFrame(1)->GetWidth() > 0);
+	reader.Close();
+}
+
 TEST_CASE( "Invalid_Path", "[libopenshot][ffmpegreader]" )
 {
 	// Check invalid path and error details

tests/FFmpegWriter.cpp

@@ -13,6 +13,7 @@
 #include <sstream>
 #include <memory>
 #include <fstream>
+#include <QTemporaryDir>
 
 #include "openshot_catch.h"
 
@@ -43,6 +44,65 @@ AVStream* first_video_stream(AVFormatContext* format_context)
 }
 }
 
+TEST_CASE("Raw video export preserves all color planes and frame ownership",
+          "[libopenshot][ffmpegwriter][rawvideo]")
+{
+	QTemporaryDir directory;
+	REQUIRE(directory.isValid());
+	// NUT uses a different stream time base from the codec's frame rate.
+	const auto filename = GENERATE("raw.avi", "raw.nut");
+	const std::string path = directory.filePath(filename).toStdString();
+	// Multiple frames and repeated exports exercise packet/frame cleanup.
+	for (int pass = 0; pass < 2; ++pass) {
+		FFmpegWriter writer(path);
+		writer.SetVideoOptions(true, "rawvideo", Fraction(30, 1), 64, 64,
+		                       Fraction(1, 1), false, false, 1000000);
+		writer.Open();
+		for (int number = 1; number <= 3; ++number) {
+			auto frame = std::make_shared<Frame>(number, 64, 64, number == 2 ? "blue" : "red");
+			writer.WriteFrame(frame);
+		}
+		writer.Close();
+
+		AVFormatContext* input = nullptr;
+		REQUIRE(avformat_open_input(&input, path.c_str(), nullptr, nullptr) == 0);
+		std::unique_ptr<AVFormatContext, void(*)(AVFormatContext*)> input_guard(
+			input, [](AVFormatContext* context) { avformat_close_input(&context); });
+		REQUIRE(avformat_find_stream_info(input, nullptr) >= 0);
+		AVStream* stream = first_video_stream(input);
+		REQUIRE(stream != nullptr);
+		AVPacket packet = {};
+		int packets = 0;
+		while (av_read_frame(input, &packet) >= 0) {
+			if (packet.stream_index == stream->index) {
+				CHECK(packet.size == 64 * 64 * 3 / 2); // Complete YUV420P image
+				CHECK(packet.pts * av_q2d(stream->time_base) == Approx(packets / 30.0).margin(0.00001));
+				++packets;
+			}
+			av_packet_unref(&packet);
+		}
+		CHECK(packets == 3);
+		input_guard.reset();
+
+		// NUT's reported duration omits the final frame interval in this FFmpeg
+		// version; verify its packets above and use AVI for reader round trips.
+		if (std::string(filename) == "raw.nut") continue;
+		FFmpegReader reader(path);
+		reader.Open();
+		CHECK(reader.info.video_length == 3);
+		for (int number = 1; number <= 3; ++number) {
+			auto frame = reader.GetFrame(number);
+			REQUIRE(frame->GetWidth() == 64);
+			REQUIRE(frame->GetHeight() == 64);
+			const QColor color = frame->GetImage()->pixelColor(32, 32);
+			CHECK(color.green() < 10);
+			CHECK(color.red() == Approx(number == 2 ? 0 : 255).margin(10));
+			CHECK(color.blue() == Approx(number == 2 ? 255 : 0).margin(10));
+		}
+		reader.Close();
+	}
+}
+
 TEST_CASE( "Webm", "[libopenshot][ffmpegwriter]" )
 {
 	// Reader

tests/check-stability.sh

@@ -0,0 +1,21 @@
+#!/usr/bin/env bash
+# Copyright (c) 2026 OpenShot Studios, LLC
+# SPDX-License-Identifier: LGPL-3.0-or-later
+
+# Run after building the FFmpegReader and FFmpegWriter test targets.
+# Usage: bash tests/check-stability.sh [build-directory]
+set -euo pipefail
+build_dir="${1:-build}"
+export QT_QPA_PLATFORM=offscreen
+
+# Fail on invalid memory access/free and definite reader leaks. The timeout
+# also turns a lifecycle deadlock into a failure instead of hanging CI.
+timeout 120s valgrind --error-exitcode=99 --leak-check=full \
+    --errors-for-leak-kinds=definite \
+    "$build_dir/tests/openshot-FFmpegReader-test" '[lifecycle]'
+
+# Writer cleanup has known pre-existing leaks. Check invalid reads/writes and
+# double frees here without suppressions that might hide the ownership bug.
+# This command does not certify leak-free export.
+timeout 120s valgrind --error-exitcode=99 --leak-check=no \
+    "$build_dir/tests/openshot-FFmpegWriter-test" '[rawvideo]'