From b7de1a885b54c1457075f7be2294366d73991747 Mon Sep 17 00:00:00 2001 From: Jonathan Thomas Date: Mon, 28 Dec 2015 02:41:32 -0600 Subject: [PATCH] Fixed some big issues with time mapping and thread safety. Lots and lots of crashes were fixed related to this. Mostly dealing with an incorrectly sized AudioSampleBuffer when trying to combine multiple frames. --- include/Clip.h | 5 ++- src/Clip.cpp | 84 ++++++++++++++++++++++++++++++++++----------- src/FrameMapper.cpp | 12 ++----- src/Timeline.cpp | 4 --- 4 files changed, 71 insertions(+), 34 deletions(-) diff --git a/include/Clip.h b/include/Clip.h index 6b8edb50..f924691d 100644 --- a/include/Clip.h +++ b/include/Clip.h @@ -121,7 +121,7 @@ namespace openshot { bool manage_reader; /// Adjust frame number minimum value - int adjust_frame_number_minimum(long int frame_number); + long int adjust_frame_number_minimum(long int frame_number); /// Apply effects to the source frame (if any) tr1::shared_ptr apply_effects(tr1::shared_ptr frame); @@ -129,6 +129,9 @@ namespace openshot { /// Get file extension string get_file_extension(string path); + /// Get a frame object or create a blank one + tr1::shared_ptr GetOrCreateFrame(long int number); + /// Adjust the audio and image of a time mapped frame tr1::shared_ptr get_time_mapped_frame(tr1::shared_ptr frame, long int frame_number) throw(ReaderClosed); diff --git a/src/Clip.cpp b/src/Clip.cpp index fc1e8f3f..95e103e3 100644 --- a/src/Clip.cpp +++ b/src/Clip.cpp @@ -258,7 +258,7 @@ tr1::shared_ptr Clip::GetFrame(long int requested_frame) throw(ReaderClos // Now that we have re-mapped what frame number is needed, go and get the frame pointer - tr1::shared_ptr original_frame = reader->GetFrame(new_frame_number); + tr1::shared_ptr original_frame = GetOrCreateFrame(new_frame_number); // Create a new frame tr1::shared_ptr frame(new Frame(new_frame_number, 1, 1, "#000000", original_frame->GetAudioSamplesCount(), original_frame->GetAudioChannelsCount())); @@ -329,11 +329,11 @@ tr1::shared_ptr Clip::get_time_mapped_frame(tr1::shared_ptr frame, // Throw error if reader not initialized throw ReaderClosed("No Reader has been initialized for this Clip. Call Reader(*reader) before calling this method.", ""); - tr1::shared_ptr new_frame; - // Check for a valid time map curve if (time.Values.size() > 1) { + tr1::shared_ptr new_frame; + // create buffer and resampler juce::AudioSampleBuffer *samples = NULL; if (!resampler) @@ -347,7 +347,7 @@ tr1::shared_ptr Clip::get_time_mapped_frame(tr1::shared_ptr frame, new_frame = tr1::shared_ptr(new Frame(new_frame_number, 1, 1, "#000000", samples_in_frame, frame->GetAudioChannelsCount())); // Copy the image from the new frame - new_frame->AddImage(reader->GetFrame(new_frame_number)->GetImage()); + new_frame->AddImage(GetOrCreateFrame(new_frame_number)->GetImage()); // Get delta (difference in previous Y value) @@ -356,7 +356,7 @@ tr1::shared_ptr Clip::get_time_mapped_frame(tr1::shared_ptr frame, // Init audio vars int sample_rate = reader->info.sample_rate; int channels = reader->info.channels; - int number_of_samples = reader->GetFrame(new_frame_number)->GetAudioSamplesCount(); + int number_of_samples = GetOrCreateFrame(new_frame_number)->GetAudioSamplesCount(); // Determine if we are speeding up or slowing down if (time.GetRepeatFraction(frame_number).den > 1) @@ -373,7 +373,7 @@ tr1::shared_ptr Clip::get_time_mapped_frame(tr1::shared_ptr frame, // Loop through channels, and get audio samples for (int channel = 0; channel < channels; channel++) // Get the audio samples for this channel - samples->addFrom(channel, 0, reader->GetFrame(new_frame_number)->GetAudioSamples(channel), number_of_samples, 1.0f); + samples->addFrom(channel, 0, GetOrCreateFrame(new_frame_number)->GetAudioSamples(channel), number_of_samples, 1.0f); // Reverse the samples (if needed) if (!time.IsIncreasing(frame_number)) @@ -402,23 +402,28 @@ tr1::shared_ptr Clip::get_time_mapped_frame(tr1::shared_ptr frame, } else if (abs(delta) > 1 && abs(delta) < 100) { - // SPEED UP (multiple frames of audio), as long as it's not more than X frames - samples = new juce::AudioSampleBuffer(channels, number_of_samples * abs(delta)); - samples->clear(); int start = 0; - if (delta > 0) { + // SPEED UP (multiple frames of audio), as long as it's not more than X frames + int total_delta_samples = 0; + for (int delta_frame = new_frame_number - (delta - 1); delta_frame <= new_frame_number; delta_frame++) + total_delta_samples += Frame::GetSamplesPerFrame(delta_frame, reader->info.fps, reader->info.sample_rate, reader->info.channels); + + // Allocate a new sample buffer for these delta frames + samples = new juce::AudioSampleBuffer(channels, total_delta_samples); + samples->clear(); + // Loop through each frame in this delta for (int delta_frame = new_frame_number - (delta - 1); delta_frame <= new_frame_number; delta_frame++) { // buffer to hold detal samples - int number_of_delta_samples = reader->GetFrame(delta_frame)->GetAudioSamplesCount(); + int number_of_delta_samples = GetOrCreateFrame(delta_frame)->GetAudioSamplesCount(); AudioSampleBuffer* delta_samples = new juce::AudioSampleBuffer(channels, number_of_delta_samples); delta_samples->clear(); for (int channel = 0; channel < channels; channel++) - delta_samples->addFrom(channel, 0, reader->GetFrame(delta_frame)->GetAudioSamples(channel), number_of_delta_samples, 1.0f); + delta_samples->addFrom(channel, 0, GetOrCreateFrame(delta_frame)->GetAudioSamples(channel), number_of_delta_samples, 1.0f); // Reverse the samples (if needed) if (!time.IsIncreasing(frame_number)) @@ -439,16 +444,25 @@ tr1::shared_ptr Clip::get_time_mapped_frame(tr1::shared_ptr frame, } else { + // SPEED UP (multiple frames of audio), as long as it's not more than X frames + int total_delta_samples = 0; + for (int delta_frame = new_frame_number - (delta + 1); delta_frame >= new_frame_number; delta_frame--) + total_delta_samples += Frame::GetSamplesPerFrame(delta_frame, reader->info.fps, reader->info.sample_rate, reader->info.channels); + + // Allocate a new sample buffer for these delta frames + samples = new juce::AudioSampleBuffer(channels, total_delta_samples); + samples->clear(); + // Loop through each frame in this delta for (int delta_frame = new_frame_number - (delta + 1); delta_frame >= new_frame_number; delta_frame--) { // buffer to hold delta samples - int number_of_delta_samples = reader->GetFrame(delta_frame)->GetAudioSamplesCount(); + int number_of_delta_samples = GetOrCreateFrame(delta_frame)->GetAudioSamplesCount(); AudioSampleBuffer* delta_samples = new juce::AudioSampleBuffer(channels, number_of_delta_samples); delta_samples->clear(); for (int channel = 0; channel < channels; channel++) - delta_samples->addFrom(channel, 0, reader->GetFrame(delta_frame)->GetAudioSamples(channel), number_of_delta_samples, 1.0f); + delta_samples->addFrom(channel, 0, GetOrCreateFrame(delta_frame)->GetAudioSamples(channel), number_of_delta_samples, 1.0f); // Reverse the samples (if needed) if (!time.IsIncreasing(frame_number)) @@ -505,19 +519,19 @@ tr1::shared_ptr Clip::get_time_mapped_frame(tr1::shared_ptr frame, } - delete samples; - samples = NULL; + delete samples; + samples = NULL; + + // Return new time mapped frame + return new_frame; } else // Use original frame return frame; - - // Return new time mapped frame - return new_frame; } // Adjust frame number minimum value -int Clip::adjust_frame_number_minimum(long int frame_number) +long int Clip::adjust_frame_number_minimum(long int frame_number) { // Never return a frame number 0 or below if (frame_number < 1) @@ -527,6 +541,36 @@ int Clip::adjust_frame_number_minimum(long int frame_number) } +// Get or generate a blank frame +tr1::shared_ptr Clip::GetOrCreateFrame(long int number) +{ + tr1::shared_ptr new_frame; + + // Init some basic properties about this frame + int samples_in_frame = Frame::GetSamplesPerFrame(number, reader->info.fps, reader->info.sample_rate, reader->info.channels); + + try { + // Attempt to get a frame (but this could fail if a reader has just been closed) + new_frame = reader->GetFrame(number); + + // Return real frame + return new_frame; + + } catch (const ReaderClosed & e) { + // ... + } catch (const TooManySeeks & e) { + // ... + } catch (const OutOfBoundsFrame & e) { + // ... + } + + // Create blank frame + new_frame = tr1::shared_ptr(new Frame(number, reader->info.width, reader->info.height, "#000000", samples_in_frame, reader->info.channels)); + new_frame->SampleRate(reader->info.sample_rate); + new_frame->ChannelsLayout(reader->info.channel_layout); + return new_frame; +} + // Generate JSON string of this object string Clip::Json() { diff --git a/src/FrameMapper.cpp b/src/FrameMapper.cpp index 1274fb7e..6a29b269 100644 --- a/src/FrameMapper.cpp +++ b/src/FrameMapper.cpp @@ -449,7 +449,6 @@ tr1::shared_ptr FrameMapper::GetFrame(long int requested_frame) throw(Rea number_to_copy = remaining_samples; // Add samples to new frame - #pragma omp critical (openshot_adding_audio) frame->AddAudio(true, channel, samples_copied, original_frame->GetAudioSamples(channel) + mapped.Samples.sample_start, number_to_copy, 1.0); } else if (starting_frame > mapped.Samples.frame_start && starting_frame < mapped.Samples.frame_end) @@ -460,7 +459,6 @@ tr1::shared_ptr FrameMapper::GetFrame(long int requested_frame) throw(Rea number_to_copy = remaining_samples; // Add samples to new frame - #pragma omp critical (openshot_adding_audio) frame->AddAudio(true, channel, samples_copied, original_frame->GetAudioSamples(channel), number_to_copy, 1.0); } else @@ -471,7 +469,6 @@ tr1::shared_ptr FrameMapper::GetFrame(long int requested_frame) throw(Rea number_to_copy = remaining_samples; // Add samples to new frame - #pragma omp critical (openshot_adding_audio) frame->AddAudio(false, channel, samples_copied, original_frame->GetAudioSamples(channel), number_to_copy, 1.0); } } @@ -483,13 +480,12 @@ tr1::shared_ptr FrameMapper::GetFrame(long int requested_frame) throw(Rea // Resample audio on frame (if needed) if (info.has_audio && - ( info.sample_rate != frame->SampleRate() || - info.channels != frame->GetAudioChannelsCount() || - info.channel_layout != frame->ChannelsLayout())) + (info.sample_rate != frame->SampleRate() || + info.channels != frame->GetAudioChannelsCount() || + info.channel_layout != frame->ChannelsLayout())) // Resample audio and correct # of channels if needed ResampleMappedAudio(frame, mapped.Odd.Frame); - // Add frame to final cache final_cache.Add(frame->number, frame); @@ -765,7 +761,6 @@ void FrameMapper::ResampleMappedAudio(tr1::shared_ptr frame, long int ori // Resize the frame to hold the right # of channels and samples int channel_buffer_size = nb_samples; - #pragma omp critical (openshot_adding_audio) frame->ResizeAudio(info.channels, channel_buffer_size, info.sample_rate, info.channel_layout); AppendDebugMethod("FrameMapper::ResampleMappedAudio (Audio successfully resampled)", "nb_samples", nb_samples, "total_frame_samples", total_frame_samples, "info.sample_rate", info.sample_rate, "channels_in_frame", channels_in_frame, "info.channels", info.channels, "info.channel_layout", info.channel_layout); @@ -806,7 +801,6 @@ void FrameMapper::ResampleMappedAudio(tr1::shared_ptr frame, long int ori } // Add samples to frame for this channel - #pragma omp critical (openshot_adding_audio) frame->AddAudio(true, channel_filter, 0, channel_buffer, position, 1.0f); AppendDebugMethod("FrameMapper::ResampleMappedAudio (Add audio to channel)", "number of samples", position, "channel_filter", channel_filter, "", -1, "", -1, "", -1, "", -1); diff --git a/src/Timeline.cpp b/src/Timeline.cpp index 69e90b00..941428bb 100644 --- a/src/Timeline.cpp +++ b/src/Timeline.cpp @@ -264,7 +264,6 @@ void Timeline::add_layer(tr1::shared_ptr new_frame, Clip* source_clip, lo // Copy audio samples (and set initial volume). Mix samples with existing audio samples. The gains are added together, to // be sure to set the gain's correctly, so the sum does not exceed 1.0 (of audio distortion will happen). - #pragma omp critical (openshot_adding_audio) new_frame->AddAudio(false, channel, 0, source_frame->GetAudioSamples(channel), source_frame->GetAudioSamplesCount(), initial_volume); } @@ -594,9 +593,6 @@ tr1::shared_ptr Timeline::GetFrame(long int requested_frame) throw(Reader // This also opens the readers for intersecting clips, and marks non-intersecting clips as 'needs closing' vector nearby_clips = find_intersecting_clips(requested_frame, minimum_frames, true); - // TODO: OpenMP is disabled in this function, due to conditional calls the ImageMagick methods, which also - // contain OpenMP parallel regions. This is a violation of OpenMP, and causes the threads to hang in some cases. - // Set the number of threads in OpenMP omp_set_num_threads(OPEN_MP_NUM_PROCESSORS); // Allow nested OpenMP sections omp_set_nested(true);