Fixed some big performance issues (some which were causing crashes) in effects (Brightness and Saturation), and created a factory class for effects, to reduce redundent code.

include/Clip.h

@@ -43,6 +43,7 @@
 #include "Enums.h"
 #include "EffectBase.h"
 #include "Effects.h"
+#include "EffectInfo.h"
 #include "FFmpegReader.h"
 #include "Fraction.h"
 #include "FrameMapper.h"

include/EffectInfo.h

@@ -45,6 +45,8 @@ namespace openshot
 	class EffectInfo
 	{
 	public:
+		// Create an instance of an effect (factory style)
+		EffectBase* CreateEffect(string effect_type);
 
 		/// JSON methods
 		static string Json(); ///< Generate JSON string of this object

include/Timeline.h

@@ -38,6 +38,7 @@
 #include "Point.h"
 #include "EffectBase.h"
 #include "Effects.h"
+#include "EffectInfo.h"
 #include "Fraction.h"
 #include "Frame.h"
 #include "FrameMapper.h"

src/Clip.cpp

@@ -856,34 +856,16 @@ void Clip::SetJsonValue(Json::Value root) {
 			// Create Effect
 			EffectBase *e = NULL;
 
-			if (!existing_effect["type"].isNull())
-				// Init the matching effect object
-				if (existing_effect["type"].asString() == "Blur")
-					e = new Blur();
+			if (!existing_effect["type"].isNull()) {
+				// Create instance of effect
+				e = EffectInfo().CreateEffect(existing_effect["type"].asString());
 
-				else if (existing_effect["type"].asString() == "Brightness")
-					e = new Brightness();
+				// Load Json into Effect
+				e->SetJsonValue(existing_effect);
 
-				else if (existing_effect["type"].asString() == "ChromaKey")
-					e = new ChromaKey();
-
-				else if (existing_effect["type"].asString() == "Deinterlace")
-					e = new Deinterlace();
-
-				else if (existing_effect["type"].asString() == "Mask")
-					e = new Mask();
-
-				else if (existing_effect["type"].asString() == "Negate")
-					e = new Negate();
-
-				else if (existing_effect["type"].asString() == "Saturation")
-					e = new Saturation();
-
-			// Load Json into Effect
-			e->SetJsonValue(existing_effect);
-
-			// Add Effect to Timeline
-			AddEffect(e);
+				// Add Effect to Timeline
+				AddEffect(e);
+			}
 		}
 	}
 	if (!root["reader"].isNull()) // does Json contain a reader?

src/EffectInfo.cpp

@@ -38,6 +38,31 @@ string EffectInfo::Json() {
 	return JsonValue().toStyledString();
 }
 
+// Create a new effect instance
+EffectBase* EffectInfo::CreateEffect(string effect_type) {
+	// Init the matching effect object
+	if (effect_type == "Blur")
+		return new Blur();
+
+	else if (effect_type == "Brightness")
+		return new Brightness();
+
+	else if (effect_type == "ChromaKey")
+		return new ChromaKey();
+
+	else if (effect_type == "Deinterlace")
+		return new Deinterlace();
+
+	else if (effect_type == "Mask")
+		return new Mask();
+
+	else if (effect_type == "Negate")
+		return new Negate();
+
+	else if (effect_type == "Saturation")
+		return new Saturation();
+}
+
 // Generate Json::JsonValue for this object
 Json::Value EffectInfo::JsonValue() {
 

src/Timeline.cpp

@@ -920,25 +920,16 @@ void Timeline::SetJsonValue(Json::Value root) throw(InvalidFile, ReaderClosed) {
 			// Create Effect
 			EffectBase *e = NULL;
 
-			if (!existing_effect["type"].isNull())
-				// Init the matching effect object
-				if (existing_effect["type"].asString() == "ChromaKey")
-					e = new ChromaKey();
+			if (!existing_effect["type"].isNull()) {
+				// Create instance of effect
+				e = EffectInfo().CreateEffect(existing_effect["type"].asString());
 
-				else if (existing_effect["type"].asString() == "Deinterlace")
-					e = new Deinterlace();
+				// Load Json into Effect
+				e->SetJsonValue(existing_effect);
 
-				else if (existing_effect["type"].asString() == "Mask")
-					e = new Mask();
-
-				else if (existing_effect["type"].asString() == "Negate")
-					e = new Negate();
-
-			// Load Json into Effect
-			e->SetJsonValue(existing_effect);
-
-			// Add Effect to Timeline
-			AddEffect(e);
+				// Add Effect to Timeline
+				AddEffect(e);
+			}
 		}
 	}
 
@@ -1036,7 +1027,6 @@ void Timeline::apply_json_to_clips(Json::Value change) throw(InvalidJSONKey) {
 	if (existing_clip && change["key"].size() == 4 && change["key"][2] == "effects")
 	{
 		// This change is actually targetting a specific effect under a clip (and not the clip)
-		EffectBase *existing_effect = NULL;
 		Json::Value key_part = change["key"][3];
 
 		if (key_part.isObject()) {
@@ -1047,16 +1037,15 @@ void Timeline::apply_json_to_clips(Json::Value change) throw(InvalidJSONKey) {
 				string effect_id = key_part["id"].asString();
 
 				// Find matching effect in timeline (if any)
+				list<EffectBase*> effect_list = existing_clip->Effects();
 				list<EffectBase*>::iterator effect_itr;
-				for (effect_itr=existing_clip->Effects().begin(); effect_itr != existing_clip->Effects().end(); ++effect_itr)
+				for (effect_itr=effect_list.begin(); effect_itr != effect_list.end(); ++effect_itr)
 				{
 					// Get effect object from the iterator
 					EffectBase *e = (*effect_itr);
 					if (e->Id() == effect_id) {
-						existing_effect = e;
-
 						// Apply the change to the effect directly
-						apply_json_to_effects(change, existing_effect);
+						apply_json_to_effects(change, e);
 						return; // effect found, don't update clip
 					}
 				}
@@ -1145,26 +1134,7 @@ void Timeline::apply_json_to_effects(Json::Value change, EffectBase* existing_ef
 		EffectBase *e = NULL;
 
 		// Init the matching effect object
-		if (effect_type == "Blur")
-			e = new Blur();
-
-		else if (effect_type == "Brightness")
-			e = new Brightness();
-
-		else if (effect_type == "ChromaKey")
-			e = new ChromaKey();
-
-		else if (effect_type == "Deinterlace")
-			e = new Deinterlace();
-
-		else if (effect_type == "Mask")
-			e = new Mask();
-
-		else if (effect_type == "Negate")
-			e = new Negate();
-
-		else if (effect_type == "Saturation")
-			e = new Saturation();
+		e = EffectInfo().CreateEffect(effect_type);
 
 		// Load Json into Effect
 		e->SetJsonValue(change["value"]);

src/effects/Brightness.cpp

@@ -76,6 +76,10 @@ tr1::shared_ptr<Frame> Brightness::GetFrame(tr1::shared_ptr<Frame> frame, long i
 	// Get the frame's image
 	tr1::shared_ptr<QImage> frame_image = frame->GetImage();
 
+	// Get keyframe values for this frame
+	float brightness_value = brightness.GetValue(frame_number);
+	float contrast_value = contrast.GetValue(frame_number);
+
 	// Loop through pixels
 	unsigned char *pixels = (unsigned char *) frame_image->bits();
 	for (int pixel = 0, byte_index=0; pixel < frame_image->width() * frame_image->height(); pixel++, byte_index+=4)
@@ -87,15 +91,15 @@ tr1::shared_ptr<Frame> Brightness::GetFrame(tr1::shared_ptr<Frame> frame, long i
 		int A = pixels[byte_index + 3];
 
 		// Adjust the contrast
-		int factor = (259 * (contrast.GetValue(frame_number) + 255)) / (255 * (259 - contrast.GetValue(frame_number)));
+		int factor = (259 * (contrast_value + 255)) / (255 * (259 - contrast_value));
 		R = constrain((factor * (R - 128)) + 128);
 		G = constrain((factor * (G - 128)) + 128);
 		B = constrain((factor * (B - 128)) + 128);
 
 		// Adjust the brightness
-		R += (255 * brightness.GetValue(frame_number));
-		G += (255 * brightness.GetValue(frame_number));
-		B += (255 * brightness.GetValue(frame_number));
+		R += (255 * brightness_value);
+		G += (255 * brightness_value);
+		B += (255 * brightness_value);
 
 		// Constrain the value from 0 to 255
 		R = constrain(R);

src/effects/Saturation.cpp

@@ -75,6 +75,12 @@ tr1::shared_ptr<Frame> Saturation::GetFrame(tr1::shared_ptr<Frame> frame, long i
 	// Get the frame's image
 	tr1::shared_ptr<QImage> frame_image = frame->GetImage();
 
+	if (!frame_image)
+		return frame;
+
+	// Get keyframe values for this frame
+	float saturation_value = saturation.GetValue(frame_number);
+
 	// Constants used for color saturation formula
 	double pR = .299;
 	double pG = .587;
@@ -96,9 +102,9 @@ tr1::shared_ptr<Frame> Saturation::GetFrame(tr1::shared_ptr<Frame> frame, long i
 						 (B * B * pB) );
 
 		// Adjust the saturation
-		R = p + (R - p) * saturation.GetValue(frame_number);
-		G = p + (G - p) * saturation.GetValue(frame_number);
-		B = p + (B - p) * saturation.GetValue(frame_number);
+		R = p + (R - p) * saturation_value;
+		G = p + (G - p) * saturation_value;
+		B = p + (B - p) * saturation_value;
 
 		// Constrain the value from 0 to 255
 		R = constrain(R);