Fixing regression in Clip::init_reader_rotation() function, which could sometimes override scale_x and scale_y, when a rotation keyframe contained more than 1 point.

src/Clip.cpp

@@ -144,36 +144,48 @@ void Clip::init_reader_settings() {
 }
 
 void Clip::init_reader_rotation() {
-	// Don't init rotation if clip already has keyframes.
-	if (rotation.GetCount() > 0)
+	// Only apply metadata rotation if clip rotation has not been explicitly set.
+	if (rotation.GetCount() > 0 || !reader)
+		return;
+
+	const auto rotate_meta = reader->info.metadata.find("rotate");
+	if (rotate_meta == reader->info.metadata.end())
 		return;
 
-	// Get rotation from metadata (if any)
 	float rotate_angle = 0.0f;
-	if (reader && reader->info.metadata.count("rotate") > 0) {
-		try {
-			rotate_angle = strtof(reader->info.metadata["rotate"].c_str(), nullptr);
-		} catch (const std::exception& e) {
-			// Leave rotate_angle at default 0.0f
-		}
+	try {
+		rotate_angle = strtof(rotate_meta->second.c_str(), nullptr);
+	} catch (const std::exception& e) {
+		return; // ignore invalid metadata
 	}
+
 	rotation = Keyframe(rotate_angle);
 
-	// Compute uniform scale factors for rotated video.
-	// Assume reader->info.width and reader->info.height are the clip's natural dimensions.
+	// Do not overwrite user-authored scale curves.
+	auto has_default_scale = [](const Keyframe& kf) {
+		return kf.GetCount() == 1 && fabs(kf.GetPoint(0).co.Y - 1.0) < 0.00001;
+	};
+	if (!has_default_scale(scale_x) || !has_default_scale(scale_y))
+		return;
+
+	// No need to adjust scaling when the metadata rotation is effectively zero.
+	if (fabs(rotate_angle) < 0.0001f)
+		return;
+
 	float w = static_cast<float>(reader->info.width);
 	float h = static_cast<float>(reader->info.height);
-	float rad = rotate_angle * M_PI / 180.0f;
+	if (w <= 0.0f || h <= 0.0f)
+		return;
+
+	float rad = rotate_angle * static_cast<float>(M_PI) / 180.0f;
 
-	// Calculate the dimensions of the bounding box for the rotated clip.
 	float new_width  = fabs(w * cos(rad)) + fabs(h * sin(rad));
 	float new_height = fabs(w * sin(rad)) + fabs(h * cos(rad));
+	if (new_width <= 0.0f || new_height <= 0.0f)
+		return;
 
-	// To have the rotated clip appear the same size as the unrotated clip,
-	// compute a uniform scale factor S that brings the bounding box back to (w, h).
 	float uniform_scale = std::min(w / new_width, h / new_height);
 
-	// Set scale keyframes uniformly.
 	scale_x = Keyframe(uniform_scale);
 	scale_y = Keyframe(uniform_scale);
 }

tests/Clip.cpp

@@ -182,6 +182,47 @@ TEST_CASE( "properties", "[libopenshot][clip]" )
 	delete reader;
 }
 
+TEST_CASE( "Metadata rotation does not override manual scaling", "[libopenshot][clip]" )
+{
+	DummyReader reader(Fraction(24, 1), 640, 480, 48000, 2, 5.0f);
+	Clip clip;
+	clip.scale_x = Keyframe(0.5);
+	clip.scale_y = Keyframe(0.5);
+
+	clip.Reader(&reader);
+
+	REQUIRE(clip.rotation.GetCount() == 0);
+	CHECK(clip.scale_x.GetPoint(0).co.Y == Approx(0.5).margin(0.00001));
+	CHECK(clip.scale_y.GetPoint(0).co.Y == Approx(0.5).margin(0.00001));
+}
+
+TEST_CASE( "Metadata rotation scales only default clips", "[libopenshot][clip]" )
+{
+	DummyReader rotated(Fraction(24, 1), 640, 480, 48000, 2, 5.0f);
+	rotated.info.metadata["rotate"] = "90";
+
+	Clip auto_clip;
+	auto_clip.Reader(&rotated);
+
+	REQUIRE(auto_clip.rotation.GetCount() == 1);
+	CHECK(auto_clip.rotation.GetPoint(0).co.Y == Approx(90.0).margin(0.00001));
+	CHECK(auto_clip.scale_x.GetPoint(0).co.Y == Approx(0.75).margin(0.00001));
+	CHECK(auto_clip.scale_y.GetPoint(0).co.Y == Approx(0.75).margin(0.00001));
+
+	DummyReader rotated_custom(Fraction(24, 1), 640, 480, 48000, 2, 5.0f);
+	rotated_custom.info.metadata["rotate"] = "90";
+
+	Clip custom_clip;
+	custom_clip.scale_x = Keyframe(0.5);
+	custom_clip.scale_y = Keyframe(0.5);
+	custom_clip.Reader(&rotated_custom);
+
+	REQUIRE(custom_clip.rotation.GetCount() == 1);
+	CHECK(custom_clip.rotation.GetPoint(0).co.Y == Approx(90.0).margin(0.00001));
+	CHECK(custom_clip.scale_x.GetPoint(0).co.Y == Approx(0.5).margin(0.00001));
+	CHECK(custom_clip.scale_y.GetPoint(0).co.Y == Approx(0.5).margin(0.00001));
+}
+
 TEST_CASE( "effects", "[libopenshot][clip]" )
 {
 	// Load clip with video
@@ -1118,4 +1159,4 @@ TEST_CASE("Reverse time curve (sample-exact, no resampling)", "[libopenshot][cli
 	clip.Close();
 	r.Close();
 	cache.Clear();
-}
+}