From af9a4893ed17005ff3c8db651b712b20fc6cf2ae Mon Sep 17 00:00:00 2001 From: Jonathan Thomas Date: Mon, 24 Nov 2025 18:33:25 -0600 Subject: [PATCH] Fixing regression inside AudioWaveformer so it uses a proper detached reader, and no longer mutates the Clip's reader (preventing a bug which caused our video to disappear when waveforms were used on the clip) --- src/AudioWaveformer.cpp | 50 ++++++++++++++++++++++++++++++--------- src/AudioWaveformer.h | 5 ++++ tests/AudioWaveformer.cpp | 41 +++++++++++++++++++++++--------- 3 files changed, 74 insertions(+), 22 deletions(-) diff --git a/src/AudioWaveformer.cpp b/src/AudioWaveformer.cpp index a7e98274..424dc2fa 100644 --- a/src/AudioWaveformer.cpp +++ b/src/AudioWaveformer.cpp @@ -16,6 +16,7 @@ #include #include +#include #include #include @@ -31,7 +32,11 @@ using namespace openshot; // Default constructor -AudioWaveformer::AudioWaveformer(ReaderBase* new_reader) : reader(new_reader) +AudioWaveformer::AudioWaveformer(ReaderBase* new_reader) : + reader(new_reader), + detached_reader(nullptr), + resolved_reader(nullptr), + source_initialized(false) { } @@ -50,7 +55,8 @@ AudioWaveformData AudioWaveformer::ExtractSamples(int channel, int num_per_secon return data; } - ReaderBase* source = ResolveSourceReader(reader); + ReaderBase* source = ResolveWaveformReader(); + Fraction source_fps = ResolveSourceFPS(source); AudioWaveformData base = ExtractSamplesFromReader(source, channel, num_per_second, false); @@ -212,12 +218,9 @@ AudioWaveformData AudioWaveformer::ExtractSamplesFromReader(ReaderBase* source_r } // Open reader (if needed) - bool does_reader_have_video = source_reader->info.has_video; if (!source_reader->IsOpen()) { source_reader->Open(); } - // Disable video for faster processing - source_reader->info.has_video = false; const auto retry_delay = std::chrono::milliseconds(100); const auto max_wait_for_open = std::chrono::milliseconds(3000); @@ -270,18 +273,15 @@ AudioWaveformData AudioWaveformer::ExtractSamplesFromReader(ReaderBase* source_r } if (!source_reader->info.has_audio) { - source_reader->info.has_video = does_reader_have_video; return data; } int total_samples = static_cast(std::ceil(reader_duration * num_per_second)); if (total_samples <= 0 || source_reader->info.channels == 0) { - source_reader->info.has_video = does_reader_have_video; return data; } if (channel != -1 && (channel < 0 || channel >= source_reader->info.channels)) { - source_reader->info.has_video = does_reader_have_video; return data; } @@ -348,7 +348,6 @@ AudioWaveformData AudioWaveformer::ExtractSamplesFromReader(ReaderBase* source_r } } } catch (...) { - source_reader->info.has_video = does_reader_have_video; throw; } @@ -369,8 +368,6 @@ AudioWaveformData AudioWaveformer::ExtractSamplesFromReader(ReaderBase* source_r data.scale(total_samples, scale); } - source_reader->info.has_video = does_reader_have_video; - return data; } @@ -400,3 +397,34 @@ Fraction AudioWaveformer::ResolveSourceFPS(ReaderBase* source_reader) { } return source_reader->info.fps; } + +// Resolve and cache the reader used for waveform extraction (prefer a detached FFmpegReader clone) +ReaderBase* AudioWaveformer::ResolveWaveformReader() { + if (source_initialized) { + return resolved_reader ? resolved_reader : reader; + } + source_initialized = true; + + resolved_reader = ResolveSourceReader(reader); + + // Prefer a detached, audio-only FFmpegReader clone so we never mutate the live reader used for preview. + if (auto ff_reader = dynamic_cast(resolved_reader)) { + const Json::Value ff_json = ff_reader->JsonValue(); + const std::string path = ff_json.get("path", "").asString(); + if (!path.empty()) { + try { + auto clone = std::make_unique(path, false); + clone->SetJsonValue(ff_json); + clone->info.has_video = false; // explicitly audio-only for waveform extraction + detached_reader = std::move(clone); + resolved_reader = detached_reader.get(); + } catch (...) { + // Fall back to using the original reader if cloning fails + detached_reader.reset(); + resolved_reader = ResolveSourceReader(reader); + } + } + } + + return resolved_reader ? resolved_reader : reader; +} diff --git a/src/AudioWaveformer.h b/src/AudioWaveformer.h index 787949ff..73ed9566 100644 --- a/src/AudioWaveformer.h +++ b/src/AudioWaveformer.h @@ -17,6 +17,7 @@ #include "Frame.h" #include "KeyFrame.h" #include "Fraction.h" +#include #include #include @@ -82,6 +83,9 @@ namespace openshot { class AudioWaveformer { private: ReaderBase* reader; + std::unique_ptr detached_reader; ///< Optional detached reader clone for waveform extraction + ReaderBase* resolved_reader = nullptr; ///< Cached pointer to the reader used for extraction + bool source_initialized = false; public: /// Default constructor @@ -123,6 +127,7 @@ namespace openshot { AudioWaveformData ExtractSamplesFromReader(openshot::ReaderBase* source_reader, int channel, int num_per_second, bool normalize); openshot::ReaderBase* ResolveSourceReader(openshot::ReaderBase* source_reader); openshot::Fraction ResolveSourceFPS(openshot::ReaderBase* source_reader); + openshot::ReaderBase* ResolveWaveformReader(); }; } diff --git a/tests/AudioWaveformer.cpp b/tests/AudioWaveformer.cpp index 18118775..7327c383 100644 --- a/tests/AudioWaveformer.cpp +++ b/tests/AudioWaveformer.cpp @@ -138,6 +138,31 @@ TEST_CASE( "Channel selection returns data and rejects invalid channel", "[libop r.Close(); } +TEST_CASE( "Waveform extraction does not mutate source reader video flag", "[libopenshot][audiowaveformer][mutation]" ) +{ + std::stringstream path; + path << TEST_MEDIA_PATH << "sintel_trailer-720p.mp4"; + FFmpegReader reader(path.str()); + Clip clip(&reader); + clip.Open(); + + const bool original_has_video_clip = clip.Reader()->info.has_video; + const bool original_has_video_reader = reader.info.has_video; + REQUIRE(original_has_video_clip == original_has_video_reader); + REQUIRE(original_has_video_reader); + + AudioWaveformer waveformer(&clip); + AudioWaveformData waveform = waveformer.ExtractSamples(-1, 5, false); + + // Extraction should not flip has_video on the live reader/clip + CHECK_FALSE(waveform.rms_samples.empty()); + CHECK(clip.Reader()->info.has_video == original_has_video_clip); + CHECK(reader.info.has_video == original_has_video_reader); + + clip.Close(); + reader.Close(); +} + TEST_CASE( "Extract waveform waits for reader reopen", "[libopenshot][audiowaveformer][stability]" ) { @@ -166,7 +191,7 @@ TEST_CASE( "Extract waveform waits for reader reopen", "[libopenshot][audiowavef reader.Close(); } -TEST_CASE( "Extract waveform times out when reader stays closed", "[libopenshot][audiowaveformer][stability]" ) +TEST_CASE( "Extract waveform continues if caller closes original reader", "[libopenshot][audiowaveformer][stability]" ) { std::stringstream path; path << TEST_MEDIA_PATH << "sintel_trailer-720p.mp4"; @@ -180,19 +205,13 @@ TEST_CASE( "Extract waveform times out when reader stays closed", "[libopenshot] return waveformer.ExtractSamples(-1, samples_per_second, false); }); + // Closing the caller's reader should not affect a detached clone used for waveform extraction. std::this_thread::sleep_for(std::chrono::milliseconds(50)); reader.Close(); - const auto start = std::chrono::steady_clock::now(); - try { - (void) future_waveform.get(); - FAIL("Expected ReaderClosed to be thrown after timeout"); - } catch (const openshot::ReaderClosed&) { - const auto elapsed = std::chrono::steady_clock::now() - start; - const auto elapsed_ms = std::chrono::duration_cast(elapsed); - CHECK(elapsed_ms.count() >= 2900); - CHECK(elapsed_ms.count() < 4500); - } + AudioWaveformData waveform; + REQUIRE_NOTHROW(waveform = future_waveform.get()); + CHECK_FALSE(waveform.rms_samples.empty()); reader.Close(); }