From 1ee6dbe33446b22d43caa937104684f278640123 Mon Sep 17 00:00:00 2001 From: Jonathan Thomas Date: Sat, 19 Sep 2026 15:38:20 -0500 Subject: [PATCH] Fix reader lifecycle races and raw-video packet ownership --- .github/workflows/ci.yml | 7 +++++ src/FFmpegReader.cpp | 8 +++--- src/FFmpegWriter.cpp | 46 ++++++++++++++---------------- tests/FFmpegReader.cpp | 56 +++++++++++++++++++++++++++++++++++++ tests/FFmpegWriter.cpp | 60 ++++++++++++++++++++++++++++++++++++++++ tests/check-stability.sh | 21 ++++++++++++++ 6 files changed, 169 insertions(+), 29 deletions(-) create mode 100644 tests/check-stability.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8b7d60ef6..add57e98a 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/src/FFmpegReader.cpp b/src/FFmpegReader.cpp index a7fa7c123..f6564a55e 100644 --- a/src/FFmpegReader.cpp +++ b/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 lock(getFrameMutex); // Open reader if not already open if (!is_open) { - // Prevent async calls to the following code - const std::lock_guard 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 lock(getFrameMutex); // Close all objects, if reader is 'open' if (is_open) { - // Prevent async calls to the following code - const std::lock_guard lock(getFrameMutex); // Mark as "closed" is_open = false; diff --git a/src/FFmpegWriter.cpp b/src/FFmpegWriter.cpp index 12c477d09..a9adb9e04 100644 --- a/src/FFmpegWriter.cpp +++ b/src/FFmpegWriter.cpp @@ -2337,27 +2337,27 @@ bool FFmpegWriter::write_video_packet(std::shared_ptr 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(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 [" @@ -2365,10 +2365,6 @@ bool FFmpegWriter::write_video_packet(std::shared_ptr frame, AVFrame *fra "error_code", error_code); return false; } - - // Deallocate packet - AV_FREE_PACKET(pkt); - } else { diff --git a/tests/FFmpegReader.cpp b/tests/FFmpegReader.cpp index e4e2d5882..4efd55236 100644 --- a/tests/FFmpegReader.cpp +++ b/tests/FFmpegReader.cpp @@ -19,6 +19,8 @@ #include #include #include +#include +#include #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 lock(reader.getFrameMutex); + std::promise 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 diff --git a/tests/FFmpegWriter.cpp b/tests/FFmpegWriter.cpp index cb55a529a..87c318d06 100644 --- a/tests/FFmpegWriter.cpp +++ b/tests/FFmpegWriter.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #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(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 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 diff --git a/tests/check-stability.sh b/tests/check-stability.sh new file mode 100644 index 000000000..8a2cc3bfd --- /dev/null +++ b/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]'