Limit values that we use for <ScreenAspectRatio> tags in Interop

CPLs to the list allowed by the standard.

4 files changed, 80 insertions, 25 deletions

diff --git a/src/raw_convert.h b/src/raw_convert.h
index 90737c0c..142763f2 100644
--- a/src/raw_convert.h
+++ b/src/raw_convert.h
@@ -30,11 +30,14 @@ namespace dcp {
  */
 template <typename P, typename Q>
 P
-raw_convert (Q v, int precision = 16)
+raw_convert (Q v, int precision = 16, bool fixed = false)
 {
 	std::stringstream s;
 	s.imbue (std::locale::classic ());
 	s << std::setprecision (precision);
+	if (fixed) {
+		s << std::fixed;
+	}
 	s << v;
 	P r;
 	s >> r;
diff --git a/src/reel_picture_asset.cc b/src/reel_picture_asset.cc
index 096d3476..36ccb810 100644
--- a/src/reel_picture_asset.cc
+++ b/src/reel_picture_asset.cc
@@ -1,5 +1,5 @@
 /*
-    Copyright (C) 2014-2015 Carl Hetherington <cth@carlh.net>
+    Copyright (C) 2014-2016 Carl Hetherington <cth@carlh.net>
 
     This program is free software; you can redistribute it and/or modify
     it under the terms of the GNU General Public License as published by
@@ -24,16 +24,19 @@
 #include "reel_picture_asset.h"
 #include "picture_asset.h"
 #include "dcp_assert.h"
+#include "raw_convert.h"
 #include "compose.hpp"
 #include <libcxml/cxml.h>
 #include <libxml++/libxml++.h>
 #include <iomanip>
+#include <cmath>
 
 using std::bad_cast;
 using std::string;
 using std::stringstream;
 using boost::shared_ptr;
 using boost::dynamic_pointer_cast;
+using boost::optional;
 using namespace dcp;
 
 ReelPictureAsset::ReelPictureAsset ()
@@ -80,9 +83,26 @@ ReelPictureAsset::write_to_cpl (xmlpp::Node* node, Standard standard) const
 
 	mp->add_child ("FrameRate")->add_child_text (String::compose ("%1 %2", _frame_rate.numerator, _frame_rate.denominator));
 	if (standard == INTEROP) {
-		stringstream s;
-		s << std::fixed << std::setprecision (2) << (float (_screen_aspect_ratio.numerator) / _screen_aspect_ratio.denominator);
-		mp->add_child ("ScreenAspectRatio")->add_child_text (s.str ());
+
+		/* Allowed values for this tag from the standard */
+		float allowed[] = { 1.33, 1.66, 1.77, 1.85, 2.00, 2.39 };
+		int const num_allowed = sizeof(allowed) / sizeof(float);
+
+		/* Actual ratio */
+		float ratio = float (_screen_aspect_ratio.numerator) / _screen_aspect_ratio.denominator;
+
+		/* Pick the closest and use that */
+		optional<float> closest;
+		optional<float> error;
+		for (int i = 0; i < num_allowed; ++i) {
+			float const e = fabsf (allowed[i] - ratio);
+			if (!closest || e < error.get()) {
+				closest = allowed[i];
+				error = e;
+			}
+		}
+
+		mp->add_child ("ScreenAspectRatio")->add_child_text (raw_convert<string> (closest.get(), 2, true));
 	} else {
 		mp->add_child ("ScreenAspectRatio")->add_child_text (
 			String::compose ("%1 %2", _screen_aspect_ratio.numerator, _screen_aspect_ratio.denominator)
diff --git a/src/subtitle_string.h b/src/subtitle_string.h
index 2317af5f..eaea94c0 100644
--- a/src/subtitle_string.h
+++ b/src/subtitle_string.h
@@ -89,7 +89,8 @@ public:
 		return _h_align;
 	}
 
-	/** @return vertical position as a proportion of the screen height from the top
+	/** @return vertical position as a proportion of the screen height from the
+	 *  vertical alignment point.
 	 *  (between 0 and 1)
 	 */
 	float v_position () const {
diff --git a/test/cpl_sar_test.cc b/test/cpl_sar_test.cc
index 3f5b07d1..095920ec 100644
--- a/test/cpl_sar_test.cc
+++ b/test/cpl_sar_test.cc
@@ -24,10 +24,25 @@
 #include <libxml++/libxml++.h>
 #include <boost/test/unit_test.hpp>
 
+using std::string;
 using boost::shared_ptr;
 
+static void
+check (shared_ptr<dcp::ReelMonoPictureAsset> pa, dcp::Fraction far, string sar)
+{
+	pa->set_screen_aspect_ratio (far);
+	xmlpp::Document doc;
+	xmlpp::Element* el = doc.create_root_node ("Test");
+	pa->write_to_cpl (el, dcp::INTEROP);
+
+	cxml::Node node (el);
+	BOOST_CHECK_EQUAL (node.node_child("MainPicture")->string_child ("ScreenAspectRatio"), sar);
+}
+
 /** Test for a reported bug where <ScreenAspectRatio> in Interop files uses
  *  excessive decimal places and (sometimes) the wrong decimal point character.
+ *  Also check that we correctly use one of the allowed <ScreenAspectRatio>
+ *  values with Interop.
  */
 BOOST_AUTO_TEST_CASE (cpl_sar)
 {
@@ -38,23 +53,39 @@ BOOST_AUTO_TEST_CASE (cpl_sar)
 			)
 		);
 
-	{
-		pa->set_screen_aspect_ratio (dcp::Fraction (1998, 1080));
-		xmlpp::Document doc;
-		xmlpp::Element* el = doc.create_root_node ("Test");
-		pa->write_to_cpl (el, dcp::INTEROP);
-
-		cxml::Node node (el);
-		BOOST_CHECK_EQUAL (node.node_child("MainPicture")->string_child ("ScreenAspectRatio"), "1.85");
-	}
-
-	{
-		pa->set_screen_aspect_ratio (dcp::Fraction (2048, 858));
-		xmlpp::Document doc;
-		xmlpp::Element* el = doc.create_root_node ("Test");
-		pa->write_to_cpl (el, dcp::INTEROP);
-
-		cxml::Node node (el);
-		BOOST_CHECK_EQUAL (node.node_child("MainPicture")->string_child ("ScreenAspectRatio"), "2.39");
-	}
+	/* Easy ones */
+	check (pa, dcp::Fraction (1998, 1080), "1.85");
+	check (pa, dcp::Fraction (2048, 858), "2.39");
+
+	/* Check the use of the allowed values */
+
+	/* Just less then, equal to and just more than 1.33 */
+	check (pa, dcp::Fraction (1200, 1000), "1.33");
+	check (pa, dcp::Fraction (1330, 1000), "1.33");
+	check (pa, dcp::Fraction (1430, 1000), "1.33");
+
+	/* Same for 1.66 */
+	check (pa, dcp::Fraction (1600, 1000), "1.66");
+	check (pa, dcp::Fraction (1660, 1000), "1.66");
+	check (pa, dcp::Fraction (1670, 1000), "1.66");
+
+	/* 1.77 */
+	check (pa, dcp::Fraction (1750, 1000), "1.77");
+	check (pa, dcp::Fraction (1770, 1000), "1.77");
+	check (pa, dcp::Fraction (1800, 1000), "1.77");
+
+	/* 1.85 */
+	check (pa, dcp::Fraction (1820, 1000), "1.85");
+	check (pa, dcp::Fraction (1850, 1000), "1.85");
+	check (pa, dcp::Fraction (1910, 1000), "1.85");
+
+	/* 2.00 */
+	check (pa, dcp::Fraction (1999, 1000), "2.00");
+	check (pa, dcp::Fraction (2000, 1000), "2.00");
+	check (pa, dcp::Fraction (2001, 1000), "2.00");
+
+	/* 2.39 */
+	check (pa, dcp::Fraction (2350, 1000), "2.39");
+	check (pa, dcp::Fraction (2390, 1000), "2.39");
+	check (pa, dcp::Fraction (2500, 1000), "2.39");
 }