Fixed a huge bug when closing a reader and re-opening it. Added some new unit tests to test for that bug, as well as check the ordering of layers and effects. Improved opening and closing of openshot::Clip->Reader() to better support multiple threads.

include/Timeline.h

@@ -156,6 +156,14 @@ namespace openshot {
 		/// Calculate time of a frame number, based on a framerate
 		float calculate_time(int number, Fraction rate);
 
+		/// Find intersecting (or non-intersecting) openshot::Clip objects
+		///
+		/// @returns A list of openshot::Clip objects
+		/// @param requested_frame The frame number that is requested.
+		/// @param number_of_frames The number of frames to check
+		/// @param include Include or Exclude intersecting clips
+		list<Clip*> find_intersecting_clips(int requested_frame, int number_of_frames, bool include);
+
 		/// Apply effects to the source frame (if any)
 		tr1::shared_ptr<Frame> apply_effects(tr1::shared_ptr<Frame> frame, int timeline_frame_number, int layer);
 

src/FFmpegReader.cpp

@@ -237,6 +237,8 @@ void FFmpegReader::Close()
 		// Clear processed lists
 		processed_video_frames.clear();
 		processed_audio_frames.clear();
+		processing_video_frames.clear();
+		processing_audio_frames.clear();
 
 		// Clear debug json
 		debug_root.clear();
@@ -247,7 +249,12 @@ void FFmpegReader::Close()
 
 		// Mark as "closed"
 		is_open = false;
+
+		// Reset some variables
 		last_frame = 0;
+		largest_frame_processed = 0;
+		seek_audio_frame_found = 0;
+		seek_video_frame_found = 0;
 	}
 }
 

src/Timeline.cpp

@@ -454,9 +454,6 @@ tr1::shared_ptr<Frame> Timeline::GetFrame(int requested_frame) throw(ReaderClose
 		#pragma omp critical (debug_output)
 		AppendDebugMethod("Timeline::GetFrame (Generating frame)", "requested_frame", requested_frame, "", -1, "", -1, "", -1, "", -1, "", -1);
 
-		// Re-Sort Clips (since they likely changed)
-		SortClips();
-
 		// Minimum number of frames to process (for performance reasons)
 		int minimum_frames = OPEN_MP_NUM_PROCESSORS;
 
@@ -473,6 +470,10 @@ tr1::shared_ptr<Frame> Timeline::GetFrame(int requested_frame) throw(ReaderClose
 				#pragma omp critical (debug_output)
 				AppendDebugMethod("Timeline::GetFrame (Loop through frames)", "requested_frame", requested_frame, "minimum_frames", minimum_frames, "", -1, "", -1, "", -1, "", -1);
 
+				// Get a list of clips that intersect with the requested section of timeline
+				// This also opens the readers for intersecting clips, and marks non-intersecting clips as 'needs closing'
+				list<Clip*> nearby_clips = find_intersecting_clips(requested_frame, minimum_frames, true);
+
 				// Loop through all requested frames
 				for (int frame_number = requested_frame; frame_number < requested_frame + minimum_frames; frame_number++)
 				{
@@ -488,9 +489,9 @@ tr1::shared_ptr<Frame> Timeline::GetFrame(int requested_frame) throw(ReaderClose
 						#pragma omp critical (debug_output)
 						AppendDebugMethod("Timeline::GetFrame (Loop through clips)", "frame_number", frame_number, "requested_time", requested_time, "clips.size()", clips.size(), "", -1, "", -1, "", -1);
 
-						// Find Clips at this time
+						// Find Clips near this time
 						list<Clip*>::iterator clip_itr;
-						for (clip_itr=clips.begin(); clip_itr != clips.end(); ++clip_itr)
+						for (clip_itr=nearby_clips.begin(); clip_itr != nearby_clips.end(); ++clip_itr)
 						{
 							// Get clip object from the iterator
 							Clip *clip = (*clip_itr);
@@ -503,10 +504,6 @@ tr1::shared_ptr<Frame> Timeline::GetFrame(int requested_frame) throw(ReaderClose
 							#pragma omp critical (debug_output)
 							AppendDebugMethod("Timeline::GetFrame (Does clip intersect)", "frame_number", frame_number, "requested_time", requested_time, "clip->Position()", clip->Position(), "clip_duration", clip_duration, "does_clip_intersect", does_clip_intersect, "", -1);
 
-							// Open (or schedule for closing) this clip, based on if it's intersecting or not
-							#pragma omp critical (reader_lock)
-							update_open_clips(clip, does_clip_intersect);
-
 							// Clip is visible
 							if (does_clip_intersect)
 							{
@@ -553,6 +550,54 @@ tr1::shared_ptr<Frame> Timeline::GetFrame(int requested_frame) throw(ReaderClose
 }
 
 
+// Find intersecting clips (or non intersecting clips)
+list<Clip*> Timeline::find_intersecting_clips(int requested_frame, int number_of_frames, bool include)
+{
+	// Find matching clips
+	list<Clip*> matching_clips;
+
+	// Calculate time of frame
+	float min_requested_time = calculate_time(requested_frame, info.fps);
+	float max_requested_time = calculate_time(requested_frame + (number_of_frames - 1), info.fps);
+
+	// Re-Sort Clips (since they likely changed)
+	SortClips();
+
+	// Find Clips at this time
+	list<Clip*>::iterator clip_itr;
+	for (clip_itr=clips.begin(); clip_itr != clips.end(); ++clip_itr)
+	{
+		// Get clip object from the iterator
+		Clip *clip = (*clip_itr);
+
+		// Does clip intersect the current requested time
+		float clip_duration = clip->End() - clip->Start();
+		bool does_clip_intersect = (clip->Position() <= min_requested_time && clip->Position() + clip_duration >= min_requested_time) ||
+								   (clip->Position() > min_requested_time && clip->Position() <= max_requested_time);
+
+		// Debug output
+		#pragma omp critical (debug_output)
+		AppendDebugMethod("Timeline::find_intersecting_clips (Is clip near or intersecting)", "requested_frame", requested_frame, "min_requested_time", min_requested_time, "max_requested_time", max_requested_time, "clip->Position()", clip->Position(), "clip_duration", clip_duration, "does_clip_intersect", does_clip_intersect);
+
+		// Open (or schedule for closing) this clip, based on if it's intersecting or not
+		#pragma omp critical (reader_lock)
+		update_open_clips(clip, does_clip_intersect);
+
+		// Clip is visible
+		if (does_clip_intersect && include)
+			// Add the intersecting clip
+			matching_clips.push_back(clip);
+
+		else if (!does_clip_intersect && !include)
+			// Add the non-intersecting clip
+			matching_clips.push_back(clip);
+
+	} // end clip loop
+
+	// return list
+	return matching_clips;
+}
+
 // Generate JSON string of this object
 string Timeline::Json() {
 

src/examples/Example.cpp

@@ -43,8 +43,10 @@ using namespace tr1;
 int main(int argc, char* argv[])
 {
 	// Reader
-	FFmpegReader r9("/home/jonathan/Videos/sintel-1024-surround.mp4");
+	FFmpegReader r9("/home/jonathan/apps/libopenshot/src/examples/sintel_trailer-720p.mp4");
+	r9.debug = true;
 	r9.Open();
+
 	//r9.info.has_audio = false;
 	//r9.enable_seek = false;
 	//r9.debug = true;

tests/Clip_Tests.cpp

@@ -118,7 +118,7 @@ TEST(Clip_Properties)
 		// Check for specific things
 		CHECK_CLOSE(1.0f, root["alpha"]["value"].asDouble(), 0.00001);
 		CHECK_EQUAL(true, root["alpha"]["keyframe"].asBool());
-		CHECK_EQUAL(true, root["changed"].asBool());
+		CHECK_EQUAL(true, root["changed"]["value"].asBool());
 
 	}
 	catch (exception e)
@@ -143,7 +143,7 @@ TEST(Clip_Properties)
 		// Check for specific things
 		CHECK_CLOSE(0.5f, root["alpha"]["value"].asDouble(), 0.001);
 		CHECK_EQUAL(false, root["alpha"]["keyframe"].asBool());
-		CHECK_EQUAL(true, root["changed"].asBool());
+		CHECK_EQUAL(true, root["changed"]["value"].asBool());
 
 	}
 	catch (exception e)
@@ -167,7 +167,7 @@ TEST(Clip_Properties)
 	{
 		// Check for specific things
 		CHECK_EQUAL(false, root["alpha"]["keyframe"].asBool());
-		CHECK_EQUAL(false, root["changed"].asBool());
+		CHECK_EQUAL(false, root["changed"]["value"].asBool());
 
 	}
 	catch (exception e)
@@ -192,7 +192,7 @@ TEST(Clip_Properties)
 		// Check for specific things
 		CHECK_CLOSE(0.0f, root["alpha"]["value"].asDouble(), 0.00001);
 		CHECK_EQUAL(true, root["alpha"]["keyframe"].asBool());
-		CHECK_EQUAL(true, root["changed"].asBool());
+		CHECK_EQUAL(true, root["changed"]["value"].asBool());
 
 	}
 	catch (exception e)

tests/FFmpegReader_Tests.cpp

@@ -185,3 +185,37 @@ TEST(FFmpegReader_Seek)
 
 }
 
+TEST(FFmpegReader_Multiple_Open_and_Close)
+{
+	// Create a reader
+	FFmpegReader r("../../src/examples/sintel_trailer-720p.mp4");
+	r.Open();
+
+	// Get frame that requires a seek
+	tr1::shared_ptr<Frame> f = r.GetFrame(1200);
+	CHECK_EQUAL(1200, f->number);
+
+	// Close and Re-open the reader
+	r.Close();
+	r.Open();
+
+	// Get frame
+	f = r.GetFrame(1);
+	CHECK_EQUAL(1, f->number);
+	f = r.GetFrame(250);
+	CHECK_EQUAL(250, f->number);
+
+	// Close and Re-open the reader
+	r.Close();
+	r.Open();
+
+	// Get frame
+	f = r.GetFrame(750);
+	CHECK_EQUAL(750, f->number);
+	f = r.GetFrame(1000);
+	CHECK_EQUAL(1000, f->number);
+
+	// Close reader
+	r.Close();
+}
+

tests/Timeline_Tests.cpp

@@ -194,3 +194,223 @@ TEST(Timeline_Check_Two_Track_Video)
 	// Close reader
 	t.Close();
 }
+
+TEST(Timeline_Clip_Order)
+{
+	// Create a timeline
+	Timeline t(640, 480, Fraction(30, 1), 44100, 2);
+
+	// Add some clips out of order
+	Clip clip_top("../../src/examples/front3.png");
+	clip_top.Layer(2);
+	t.AddClip(&clip_top);
+
+	Clip clip_middle("../../src/examples/front.png");
+	clip_middle.Layer(0);
+	t.AddClip(&clip_middle);
+
+	Clip clip_bottom("../../src/examples/back.png");
+	clip_bottom.Layer(1);
+	t.AddClip(&clip_bottom);
+
+	// Open Timeline
+	t.Open();
+
+	// Loop through Clips and check order (they should have been sorted into the correct order)
+	// Bottom layer to top layer, then by position.
+	list<Clip*>::iterator clip_itr;
+	list<Clip*> clips = t.Clips();
+	int counter = 0;
+	for (clip_itr=clips.begin(); clip_itr != clips.end(); ++clip_itr)
+	{
+		// Get clip object from the iterator
+		Clip *clip = (*clip_itr);
+
+		switch (counter) {
+		case 0:
+			CHECK_EQUAL(0, clip->Layer());
+			break;
+		case 1:
+			CHECK_EQUAL(1, clip->Layer());
+			break;
+		case 2:
+			CHECK_EQUAL(2, clip->Layer());
+			break;
+		}
+
+		// increment counter
+		counter++;
+	}
+
+	// Add another clip
+	Clip clip_middle1("../../src/examples/interlaced.png");
+	clip_middle1.Layer(1);
+	clip_middle1.Position(0.5);
+	t.AddClip(&clip_middle1);
+
+
+	// Loop through clips again, and re-check order
+	counter = 0;
+	clips = t.Clips();
+	for (clip_itr=clips.begin(); clip_itr != clips.end(); ++clip_itr)
+	{
+		// Get clip object from the iterator
+		Clip *clip = (*clip_itr);
+
+		switch (counter) {
+		case 0:
+			CHECK_EQUAL(0, clip->Layer());
+			break;
+		case 1:
+			CHECK_EQUAL(1, clip->Layer());
+			CHECK_CLOSE(0.0, clip->Position(), 0.0001);
+			break;
+		case 2:
+			CHECK_EQUAL(1, clip->Layer());
+			CHECK_CLOSE(0.5, clip->Position(), 0.0001);
+			break;
+		case 3:
+			CHECK_EQUAL(2, clip->Layer());
+			break;
+		}
+
+		// increment counter
+		counter++;
+	}
+
+	// Close reader
+	t.Close();
+}
+
+
+TEST(Timeline_Effect_Order)
+{
+	// Create a timeline
+	Timeline t(640, 480, Fraction(30, 1), 44100, 2);
+
+	// Add some effects out of order
+	Negate effect_top;
+	effect_top.Id("C");
+	effect_top.Layer(2);
+	t.AddEffect(&effect_top);
+
+	Negate effect_middle;
+	effect_middle.Id("A");
+	effect_middle.Layer(0);
+	t.AddEffect(&effect_middle);
+
+	Negate effect_bottom;
+	effect_bottom.Id("B");
+	effect_bottom.Layer(1);
+	t.AddEffect(&effect_bottom);
+
+	// Open Timeline
+	t.Open();
+
+	// Loop through effects and check order (they should have been sorted into the correct order)
+	// Bottom layer to top layer, then by position, and then by order.
+	list<EffectBase*>::iterator effect_itr;
+	list<EffectBase*> effects = t.Effects();
+	int counter = 0;
+	for (effect_itr=effects.begin(); effect_itr != effects.end(); ++effect_itr)
+	{
+		// Get clip object from the iterator
+		EffectBase *effect = (*effect_itr);
+
+		switch (counter) {
+		case 0:
+			CHECK_EQUAL(0, effect->Layer());
+			CHECK_EQUAL("A", effect->Id());
+			CHECK_EQUAL(0, effect->Order());
+			break;
+		case 1:
+			CHECK_EQUAL(1, effect->Layer());
+			CHECK_EQUAL("B", effect->Id());
+			CHECK_EQUAL(0, effect->Order());
+			break;
+		case 2:
+			CHECK_EQUAL(2, effect->Layer());
+			CHECK_EQUAL("C", effect->Id());
+			CHECK_EQUAL(0, effect->Order());
+			break;
+		}
+
+		// increment counter
+		counter++;
+	}
+
+	// Add some more effects out of order
+	Negate effect_top1;
+	effect_top1.Id("B-2");
+	effect_top1.Layer(1);
+	effect_top1.Position(0.5);
+	effect_top1.Order(2);
+	t.AddEffect(&effect_top1);
+
+	Negate effect_middle1;
+	effect_middle1.Id("B-3");
+	effect_middle1.Layer(1);
+	effect_middle1.Position(0.5);
+	effect_middle1.Order(1);
+	t.AddEffect(&effect_middle1);
+
+	Negate effect_bottom1;
+	effect_bottom1.Id("B-1");
+	effect_bottom1.Layer(1);
+	effect_bottom1.Position(0);
+	effect_bottom1.Order(3);
+	t.AddEffect(&effect_bottom1);
+
+
+	// Loop through effects again, and re-check order
+	effects = t.Effects();
+	counter = 0;
+	for (effect_itr=effects.begin(); effect_itr != effects.end(); ++effect_itr)
+	{
+		// Get clip object from the iterator
+		EffectBase *effect = (*effect_itr);
+
+		switch (counter) {
+		case 0:
+			CHECK_EQUAL(0, effect->Layer());
+			CHECK_EQUAL("A", effect->Id());
+			CHECK_EQUAL(0, effect->Order());
+			break;
+		case 1:
+			CHECK_EQUAL(1, effect->Layer());
+			CHECK_EQUAL("B-1", effect->Id());
+			CHECK_CLOSE(0.0, effect->Position(), 0.0001);
+			CHECK_EQUAL(3, effect->Order());
+			break;
+		case 2:
+			CHECK_EQUAL(1, effect->Layer());
+			CHECK_EQUAL("B", effect->Id());
+			CHECK_CLOSE(0.0, effect->Position(), 0.0001);
+			CHECK_EQUAL(0, effect->Order());
+			break;
+		case 3:
+			CHECK_EQUAL(1, effect->Layer());
+			CHECK_EQUAL("B-2", effect->Id());
+			CHECK_CLOSE(0.5, effect->Position(), 0.0001);
+			CHECK_EQUAL(2, effect->Order());
+			break;
+		case 4:
+			CHECK_EQUAL(1, effect->Layer());
+			CHECK_EQUAL("B-3", effect->Id());
+			CHECK_CLOSE(0.5, effect->Position(), 0.0001);
+			CHECK_EQUAL(1, effect->Order());
+			break;
+		case 5:
+			CHECK_EQUAL(2, effect->Layer());
+			CHECK_EQUAL("C", effect->Id());
+			CHECK_EQUAL(0, effect->Order());
+			break;
+		}
+
+		// increment counter
+		counter++;
+	}
+
+	// Close reader
+	t.Close();
+}