- Protect AddClip(), RemoveClip(), update_open_clips(), sort_clips(),…

… sort_effects() methods with mutex, making them thread safe

- Refactor sorting of clips & effects, and only sort these arrays when the arrays change (instead of each call to GetFrame)
- Cache max timeline duration, and make Timeline::GetMaxTime() thread safe
- New multi-threaded unit tests, which are designed to verify no seg faults on multi-threaded calls to Timeline::GetFrame(), Timeline::AddClip(), and Timeline::RemoveClip()
- New public Timeline::SortTimeline() method which is called by child Clips automatically, when certain properties are changed

src/ClipBase.cpp

@@ -11,9 +11,55 @@
 // SPDX-License-Identifier: LGPL-3.0-or-later
 
 #include "ClipBase.h"
+#include "Timeline.h"
 
 using namespace openshot;
 
+// Set position on timeline (in seconds)
+void ClipBase::Position(float value) {
+
+	position = value;
+
+	if (ParentTimeline()) {
+		// Resort timeline items (internal clips/effects arrays)
+		Timeline *parentTimeline = (Timeline *) ParentTimeline();
+		parentTimeline->SortTimeline();
+	}
+}
+
+// Set layer of clip on timeline (lower number is covered by higher numbers)
+void ClipBase::Layer(int value) {
+	layer = value;
+
+	if (ParentTimeline()) {
+		// Resort timeline items (internal clips/effects arrays)
+		Timeline *parentTimeline = (Timeline *) ParentTimeline();
+		parentTimeline->SortTimeline();
+	}
+}
+
+// Set start position (in seconds) of clip (trim start of video)
+void ClipBase::Start(float value) {
+	start = value;
+
+	if (ParentTimeline()) {
+		// Resort timeline items (internal clips/effects arrays)
+		Timeline *parentTimeline = (Timeline *) ParentTimeline();
+		parentTimeline->SortTimeline();
+	}
+}
+
+// Set end position (in seconds) of clip (trim end of video)
+void ClipBase::End(float value) {
+	end = value;
+
+	if (ParentTimeline()) {
+		// Resort timeline items (internal clips/effects arrays)
+		Timeline *parentTimeline = (Timeline *) ParentTimeline();
+		parentTimeline->SortTimeline();
+	}
+}
+
 // Generate Json::Value for this object
 Json::Value ClipBase::JsonValue() const {
 

src/ClipBase.h

@@ -86,16 +86,16 @@ namespace openshot {
 		float Position() const { return position; } ///< Get position on timeline (in seconds)
 		int Layer() const { return layer; } ///< Get layer of clip on timeline (lower number is covered by higher numbers)
 		float Start() const { return start; } ///< Get start position (in seconds) of clip (trim start of video)
-		float End() const { return end; } ///< Get end position (in seconds) of clip (trim end of video)
+		virtual float End() const { return end; } ///< Get end position (in seconds) of clip (trim end of video)
 		float Duration() const { return end - start; } ///< Get the length of this clip (in seconds)
 		openshot::TimelineBase* ParentTimeline() { return timeline; } ///< Get the associated Timeline pointer (if any)
 
 		// Set basic properties
 		void Id(std::string value) { id = value; } ///> Set the Id of this clip object
-		void Position(float value) { position = value; } ///< Set position on timeline (in seconds)
-		void Layer(int value) { layer = value; } ///< Set layer of clip on timeline (lower number is covered by higher numbers)
-		void Start(float value) { start = value; } ///< Set start position (in seconds) of clip (trim start of video)
-		void End(float value) { end = value; } ///< Set end position (in seconds) of clip (trim end of video)
+        void Position(float value); ///< Set position on timeline (in seconds)
+        void Layer(int value); ///< Set layer of clip on timeline (lower number is covered by higher numbers)
+		void Start(float value); ///< Set start position (in seconds) of clip (trim start of video)
+		virtual void End(float value); ///< Set end position (in seconds) of clip (trim end of video)
 		void ParentTimeline(openshot::TimelineBase* new_timeline) { timeline = new_timeline; } ///< Set associated Timeline pointer
 
 		// Get and Set JSON methods

src/Timeline.cpp

@@ -27,7 +27,7 @@ using namespace openshot;
 // Default Constructor for the timeline (which sets the canvas width and height)
 Timeline::Timeline(int width, int height, Fraction fps, int sample_rate, int channels, ChannelLayout channel_layout) :
 		is_open(false), auto_map_clips(true), managed_cache(true), path(""),
-		max_concurrent_frames(OPEN_MP_NUM_PROCESSORS)
+		max_concurrent_frames(OPEN_MP_NUM_PROCESSORS), max_time(0.0)
 {
 	// Create CrashHandler and Attach (incase of errors)
 	CrashHandler::Instance();
@@ -78,7 +78,7 @@ Timeline::Timeline(const ReaderInfo info) : Timeline::Timeline(
 // Constructor for the timeline (which loads a JSON structure from a file path, and initializes a timeline)
 Timeline::Timeline(const std::string& projectPath, bool convert_absolute_paths) :
 		is_open(false), auto_map_clips(true), managed_cache(true), path(projectPath),
-		max_concurrent_frames(OPEN_MP_NUM_PROCESSORS) {
+		max_concurrent_frames(OPEN_MP_NUM_PROCESSORS), max_time(0.0) {
 
 	// Create CrashHandler and Attach (incase of errors)
 	CrashHandler::Instance();
@@ -331,6 +331,9 @@ std::string Timeline::GetTrackedObjectValues(std::string id, int64_t frame_numbe
 // Add an openshot::Clip to the timeline
 void Timeline::AddClip(Clip* clip)
 {
+	// Get lock (prevent getting frames while this happens)
+	const std::lock_guard<std::recursive_mutex> guard(getFrameMutex);
+
 	// Assign timeline to clip
 	clip->ParentTimeline(this);
 
@@ -375,11 +378,17 @@ void Timeline::RemoveEffect(EffectBase* effect)
 		effect = NULL;
 		allocated_effects.erase(effect);
 	}
+
+	// Sort effects
+	sort_effects();
 }
 
 // Remove an openshot::Clip to the timeline
 void Timeline::RemoveClip(Clip* clip)
 {
+	// Get lock (prevent getting frames while this happens)
+	const std::lock_guard<std::recursive_mutex> guard(getFrameMutex);
+
 	clips.remove(clip);
 
 	// Delete clip object (if timeline allocated it)
@@ -389,6 +398,9 @@ void Timeline::RemoveClip(Clip* clip)
 		clip = NULL;
 		allocated_clips.erase(clip);
 	}
+
+	// Sort clips
+	sort_clips();
 }
 
 // Look up a clip
@@ -448,20 +460,8 @@ std::list<openshot::EffectBase*> Timeline::ClipEffects() const {
 
 // Compute the end time of the latest timeline element
 double Timeline::GetMaxTime() {
-	double last_clip = 0.0;
-	double last_effect = 0.0;
-
-	if (!clips.empty()) {
-		const auto max_clip = std::max_element(
-				clips.begin(), clips.end(), CompareClipEndFrames());
-		last_clip = (*max_clip)->Position() + (*max_clip)->Duration();
-	}
-	if (!effects.empty()) {
-		const auto max_effect = std::max_element(
-				effects.begin(), effects.end(), CompareEffectEndFrames());
-		last_effect = (*max_effect)->Position() + (*max_effect)->Duration();
-	}
-	return std::max(last_clip, last_effect);
+    // Return cached max_time variable (threadsafe)
+    return max_time;
 }
 
 // Compute the highest frame# based on the latest time and FPS
@@ -712,6 +712,9 @@ void Timeline::add_layer(std::shared_ptr<Frame> new_frame, Clip* source_clip, in
 // Update the list of 'opened' clips
 void Timeline::update_open_clips(Clip *clip, bool does_clip_intersect)
 {
+	// Get lock (prevent getting frames while this happens)
+	const std::lock_guard<std::recursive_mutex> guard(getFrameMutex);
+
 	ZmqLogger::Instance()->AppendDebugMethod(
 		"Timeline::update_open_clips (before)",
 		"does_clip_intersect", does_clip_intersect,
@@ -752,23 +755,53 @@ void Timeline::update_open_clips(Clip *clip, bool does_clip_intersect)
 		"open_clips.size()", open_clips.size());
 }
 
+// Calculate the max duration (in seconds) of the timeline, based on all the clips, and cache the value
+void Timeline::calculate_max_duration() {
+	double last_clip = 0.0;
+	double last_effect = 0.0;
+
+	if (!clips.empty()) {
+		const auto max_clip = std::max_element(
+				clips.begin(), clips.end(), CompareClipEndFrames());
+		last_clip = (*max_clip)->Position() + (*max_clip)->Duration();
+	}
+	if (!effects.empty()) {
+		const auto max_effect = std::max_element(
+				effects.begin(), effects.end(), CompareEffectEndFrames());
+		last_effect = (*max_effect)->Position() + (*max_effect)->Duration();
+	}
+	max_time = std::max(last_clip, last_effect);
+}
+
 // Sort clips by position on the timeline
 void Timeline::sort_clips()
 {
+	// Get lock (prevent getting frames while this happens)
+	const std::lock_guard<std::recursive_mutex> guard(getFrameMutex);
+
 	// Debug output
 	ZmqLogger::Instance()->AppendDebugMethod(
 		"Timeline::SortClips",
 		"clips.size()", clips.size());
 
 	// sort clips
 	clips.sort(CompareClips());
+
+	// calculate max timeline duration
+	calculate_max_duration();
 }
 
 // Sort effects by position on the timeline
 void Timeline::sort_effects()
 {
+	// Get lock (prevent getting frames while this happens)
+	const std::lock_guard<std::recursive_mutex> guard(getFrameMutex);
+
 	// sort clips
 	effects.sort(CompareEffects());
+
+	// calculate max timeline duration
+	calculate_max_duration();
 }
 
 // Clear all clips from timeline

src/Timeline.h

@@ -162,6 +162,7 @@ namespace openshot {
 		bool managed_cache; ///< Does this timeline instance manage the cache object
 		std::string path; ///< Optional path of loaded UTF-8 OpenShot JSON project file
 		int max_concurrent_frames; ///< Max concurrent frames to process at one time
+		double max_time; ///> The max duration (in seconds) of the timeline, based on all the clips
 
 		std::map<std::string, std::shared_ptr<openshot::TrackedObjectBase>> tracked_objects; ///< map of TrackedObjectBBoxes and their IDs
 
@@ -177,6 +178,9 @@ namespace openshot {
 		void apply_json_to_effects(Json::Value change, openshot::EffectBase* existing_effect); ///<Apply JSON diff to a specific effect
 		void apply_json_to_timeline(Json::Value change); ///<Apply JSON diff to timeline properties
 
+		/// Calculate the max duration (in seconds) of the timeline, based on all the clips, and cache the value
+		void calculate_max_duration();
+
 		/// Calculate time of a frame number, based on a framerate
 		double calculate_time(int64_t number, openshot::Fraction rate);
 
@@ -346,6 +350,10 @@ namespace openshot {
 		/// @brief Remove an effect from the timeline
 		/// @param effect Remove an effect from the timeline.
 		void RemoveEffect(openshot::EffectBase* effect);
+
+		/// @brief Sort all clips and effects on timeline - which affects the internal order of clips and effects arrays
+		/// This is called automatically when Clips or Effects modify the Layer(), Position(), Start(), or End().
+		void SortTimeline() { sort_clips(); sort_effects(); }
 	};
 
 }