From a8e9c95fea40ed2f6e29bb75f49f1c7dc3df9da1 Mon Sep 17 00:00:00 2001 From: Jonathan Thomas Date: Wed, 2 Mar 2022 16:24:09 -0600 Subject: [PATCH] Silence openshot::Frame audio when requesting a Clip::GetFrame() past the end of the Clip's Reader. For example, if a Clip has 1000 frames, and the user requests frame 1001, we will return the last cached openshot::Frame object, but we don't want to repeat the audio samples (causing a stutter). Any frame past the end of the reader, should always silence the audio samples. Also, fixed a few invalid comments, and added a Unit test. --- src/Clip.cpp | 12 ++++++--- src/DummyReader.cpp | 17 ++++++++---- src/DummyReader.h | 1 + tests/Clip.cpp | 60 +++++++++++++++++++++++++++++++++++++++++++ tests/DummyReader.cpp | 29 +-------------------- tests/FrameMapper.cpp | 4 +-- 6 files changed, 84 insertions(+), 39 deletions(-) diff --git a/src/Clip.cpp b/src/Clip.cpp index f0aa8075..f081d7e3 100644 --- a/src/Clip.cpp +++ b/src/Clip.cpp @@ -712,10 +712,14 @@ std::shared_ptr Clip::GetOrCreateFrame(int64_t number) // This allows a clip to modify the pixels and audio of this frame without // changing the underlying reader's frame data auto reader_copy = std::make_shared(*reader_frame.get()); - if (has_video.GetInt(number) == 0) - reader_copy->AddColor(QColor(Qt::transparent)); - if (has_audio.GetInt(number) == 0) - reader_copy->AddAudioSilence(reader_copy->GetAudioSamplesCount()); + if (has_video.GetInt(number) == 0) { + // No video, so add transparent pixels + reader_copy->AddColor(QColor(Qt::transparent)); + } + if (has_audio.GetInt(number) == 0 || number > reader->info.video_length) { + // No audio, so include silence (also, mute audio if past end of reader) + reader_copy->AddAudioSilence(reader_copy->GetAudioSamplesCount()); + } return reader_copy; } diff --git a/src/DummyReader.cpp b/src/DummyReader.cpp index f289b306..f2cfe7e6 100644 --- a/src/DummyReader.cpp +++ b/src/DummyReader.cpp @@ -47,21 +47,23 @@ void DummyReader::init(Fraction fps, int width, int height, int sample_rate, int } // Blank constructor for DummyReader, with default settings. -DummyReader::DummyReader() : dummy_cache(NULL), is_open(false) { +DummyReader::DummyReader() : dummy_cache(NULL), last_cached_frame(NULL), image_frame(NULL), is_open(false) { // Initialize important variables init(Fraction(24,1), 1280, 768, 44100, 2, 30.0); } // Constructor for DummyReader. Pass a framerate and samplerate. -DummyReader::DummyReader(Fraction fps, int width, int height, int sample_rate, int channels, float duration) : dummy_cache(NULL), is_open(false) { +DummyReader::DummyReader(Fraction fps, int width, int height, int sample_rate, int channels, float duration) : + dummy_cache(NULL), last_cached_frame(NULL), image_frame(NULL), is_open(false) { // Initialize important variables init(fps, width, height, sample_rate, channels, duration); } // Constructor which also takes a cache object -DummyReader::DummyReader(Fraction fps, int width, int height, int sample_rate, int channels, float duration, CacheBase* cache) : is_open(false) { +DummyReader::DummyReader(Fraction fps, int width, int height, int sample_rate, int channels, float duration, + CacheBase* cache) : last_cached_frame(NULL), image_frame(NULL), is_open(false) { // Initialize important variables init(fps, width, height, sample_rate, channels, duration); @@ -117,6 +119,7 @@ std::shared_ptr DummyReader::GetFrame(int64_t requested_frame) // Always return same frame (regardless of which frame number was requested) image_frame->number = requested_frame; + last_cached_frame = image_frame; return image_frame; } else if (dummy_cache_count > 0) { @@ -126,8 +129,12 @@ std::shared_ptr DummyReader::GetFrame(int64_t requested_frame) // Get a frame from the dummy cache std::shared_ptr f = dummy_cache->GetFrame(requested_frame); if (f) { - // return frame from cache (if found) - return f; + // return frame from cache (if found) + last_cached_frame = f; + return f; + } else if (last_cached_frame) { + // If available, return last cached frame + return last_cached_frame; } else { // No cached frame found throw InvalidFile("Requested frame not found. You can only access Frame numbers that exist in the Cache object.", "dummy"); diff --git a/src/DummyReader.h b/src/DummyReader.h index 2ffb893e..28044744 100644 --- a/src/DummyReader.h +++ b/src/DummyReader.h @@ -87,6 +87,7 @@ namespace openshot private: CacheBase* dummy_cache; std::shared_ptr image_frame; + std::shared_ptr last_cached_frame; bool is_open; /// Initialize variables used by constructor diff --git a/tests/Clip.cpp b/tests/Clip.cpp index b875b79c..46b60d74 100644 --- a/tests/Clip.cpp +++ b/tests/Clip.cpp @@ -20,6 +20,7 @@ #include #include "Clip.h" +#include "DummyReader.h" #include "Enums.h" #include "Exceptions.h" #include "Frame.h" @@ -277,3 +278,62 @@ TEST_CASE( "has_video", "[libopenshot][clip]" ) CHECK(i3->size() == f3_size); CHECK(i3->pixelColor(20, 20) != trans_color); } + +TEST_CASE( "access frames past reader length", "[libopenshot][clip]" ) +{ + // Create cache object to hold test frames + openshot::CacheMemory cache; + + // Let's create some test frames + for (int64_t frame_number = 1; frame_number <= 30; frame_number++) { + // Create blank frame (with specific frame #, samples, and channels) + // Sample count should be 44100 / 30 fps = 1470 samples per frame + int sample_count = 1470; + auto f = std::make_shared(frame_number, sample_count, 2); + + // Create test samples with incrementing value + float *audio_buffer = new float[sample_count]; + for (int64_t sample_number = 0; sample_number < sample_count; sample_number++) { + // Generate an incrementing audio sample value (just as an example) + audio_buffer[sample_number] = float(frame_number) + (float(sample_number) / float(sample_count)); + } + + // Add custom audio samples to Frame (bool replaceSamples, int destChannel, int destStartSample, const float* source, + f->AddAudio(true, 0, 0, audio_buffer, sample_count, 1.0); // add channel 1 + f->AddAudio(true, 1, 0, audio_buffer, sample_count, 1.0); // add channel 2 + + // Add test frame to dummy reader + cache.Add(f); + + delete[] audio_buffer; + } + + // Create a dummy reader, with a pre-existing cache + openshot::DummyReader r(openshot::Fraction(30, 1), 1920, 1080, 44100, 2, 1.0, &cache); + r.Open(); // Open the reader + + openshot::Clip c1; + c1.Reader(&r); + c1.Open(); + + // Get the last valid frame # + std::shared_ptr frame = c1.GetFrame(30); + + CHECK(frame->GetAudioSamples(0)[0] == Approx(30.0).margin(0.00001)); + CHECK(frame->GetAudioSamples(0)[600] == Approx(30.4081631).margin(0.00001)); + CHECK(frame->GetAudioSamples(0)[1200] == Approx(30.8163261).margin(0.00001)); + + // Get the +1 past the end of the reader (should be audio silence) + frame = c1.GetFrame(31); + + CHECK(frame->GetAudioSamples(0)[0] == Approx(0.0).margin(0.00001)); + CHECK(frame->GetAudioSamples(0)[600] == Approx(0.0).margin(0.00001)); + CHECK(frame->GetAudioSamples(0)[1200] == Approx(0.0).margin(0.00001)); + + // Get the +2 past the end of the reader (should be audio silence) + frame = c1.GetFrame(32); + + CHECK(frame->GetAudioSamples(0)[0] == Approx(0.0).margin(0.00001)); + CHECK(frame->GetAudioSamples(0)[600] == Approx(0.0).margin(0.00001)); + CHECK(frame->GetAudioSamples(0)[1200] == Approx(0.0).margin(0.00001)); +} \ No newline at end of file diff --git a/tests/DummyReader.cpp b/tests/DummyReader.cpp index 28d8a88b..7cc075b9 100644 --- a/tests/DummyReader.cpp +++ b/tests/DummyReader.cpp @@ -21,7 +21,6 @@ #include "Frame.h" TEST_CASE( "Default constructor", "[libopenshot][dummyreader]" ) { - // Create a default fraction (should be 1/1) openshot::DummyReader r; r.Open(); // Open the reader @@ -41,7 +40,6 @@ TEST_CASE( "Default constructor", "[libopenshot][dummyreader]" ) { } TEST_CASE( "Constructor", "[libopenshot][dummyreader]" ) { - // Create a default fraction (should be 1/1) openshot::DummyReader r(openshot::Fraction(30, 1), 1920, 1080, 44100, 2, 60.0); r.Open(); // Open the reader @@ -56,7 +54,6 @@ TEST_CASE( "Constructor", "[libopenshot][dummyreader]" ) { } TEST_CASE( "Blank_Frame", "[libopenshot][dummyreader]" ) { - // Create a default fraction (should be 1/1) openshot::DummyReader r(openshot::Fraction(30, 1), 1920, 1080, 44100, 2, 30.0); r.Open(); // Open the reader @@ -96,7 +93,7 @@ TEST_CASE( "Fake_Frame", "[libopenshot][dummyreader]" ) { delete[] audio_buffer; } - // Create a default fraction (should be 1/1) + // Create a dummy reader, with a pre-existing cache openshot::DummyReader r(openshot::Fraction(30, 1), 1920, 1080, 44100, 2, 30.0, &cache); r.Open(); // Open the reader @@ -114,30 +111,6 @@ TEST_CASE( "Fake_Frame", "[libopenshot][dummyreader]" ) { r.Close(); } -TEST_CASE( "Invalid_Fake_Frame", "[libopenshot][dummyreader]" ) { - // Create fake frames (with specific frame #, samples, and channels) - auto f1 = std::make_shared(1, 1470, 2); - auto f2 = std::make_shared(2, 1470, 2); - - // Add test frames to cache object - openshot::CacheMemory cache; - cache.Add(f1); - cache.Add(f2); - - // Create a default fraction (should be 1/1) - openshot::DummyReader r(openshot::Fraction(30, 1), 1920, 1080, 44100, 2, 30.0, &cache); - r.Open(); - - // Verify exception - CHECK(r.GetFrame(1)->number == 1); - CHECK(r.GetFrame(2)->number == 2); - CHECK_THROWS_AS(r.GetFrame(3)->number, openshot::InvalidFile); - - // Clean up - cache.Clear(); - r.Close(); -} - TEST_CASE( "Json", "[libopenshot][dummyreader]") { openshot::DummyReader r1; openshot::DummyReader r2(openshot::Fraction(24, 1), 1280, 768, 44100, 2, 30.0); diff --git a/tests/FrameMapper.cpp b/tests/FrameMapper.cpp index 65429347..7cf0e88a 100644 --- a/tests/FrameMapper.cpp +++ b/tests/FrameMapper.cpp @@ -243,7 +243,7 @@ TEST_CASE( "resample_audio_mapper", "[libopenshot][framemapper]" ) { delete[] audio_buffer; } - // Create a default fraction (should be 1/1) + // Create a dummy reader, with a pre-existing cache openshot::DummyReader r(openshot::Fraction(30, 1), 1, 1, 44100, 2, 30.0, &cache); r.Open(); // Open the reader @@ -383,7 +383,7 @@ TEST_CASE( "redistribute_samples_per_frame", "[libopenshot][framemapper]" ) { delete[] audio_buffer; } - // Create a default fraction (should be 1/1) + // Create a dummy reader, with a pre-existing cache openshot::DummyReader r(openshot::Fraction(30, 1), 1920, 1080, 44100, 2, 30.0, &cache); r.Open(); // Open the reader