Increase timebase of Time so that more frame rates are possible without rounding (#2919).

6 files changed, 107 insertions, 56 deletions

diff --git a/hacks/hz.py b/hacks/hz.py
index 78951d101..3d85d86cb 100644
--- a/hacks/hz.py
+++ b/hacks/hz.py
@@ -1,23 +1,32 @@
 import math
 
-MAX_FPS = 30
+factors = (
+    16,
+    17,
+    18,
+    19,
+    20,
+    21,
+    22,
+    23,
+    24,
+    25,
+    30,
+    48,
+    50,
+    60,
+    48000,
+    96000,
+)
 
-X = MAX_FPS
-for i in range(MAX_FPS - 1, 1, -1):
-    if (X % i) != 0:
-        X *= i
-
-for i in (36, 48, 60):
-    if (X % i) != 0:
-        X *= i
+X = 1
+for factor in factors:
+    if (X % factor):
+        X *= factor
 
 print(f"Timebase: {X}")
 print(f"Bits: {math.log2(X)}")
 
-# 5 hours
-HOURS = 5
+HOURS = 12
 print(f"{HOURS} hour film requires: {math.log2(X * HOURS * 60 * 60)} bits")
 
-for i in range(2, MAX_FPS + 1):
-    assert (X % i) == 0
-
diff --git a/src/lib/dcpomatic_time.h b/src/lib/dcpomatic_time.h
index f19463e01..235c95a43 100644
--- a/src/lib/dcpomatic_time.h
+++ b/src/lib/dcpomatic_time.h
@@ -36,6 +36,7 @@
 LIBDCP_DISABLE_WARNINGS
 #include <libxml++/libxml++.h>
 LIBDCP_ENABLE_WARNINGS
+#include <boost/multiprecision/cpp_int.hpp>
 #include <boost/optional.hpp>
 #include <cmath>
 #include <cstdio>
@@ -75,7 +76,7 @@ public:
 bool operator<=(HMSF const& a, HMSF const& b);
 
 
-/** A time in seconds, expressed as a number scaled up by Time::HZ.  We want two different
+/** A time in seconds, expressed as a number scaled up by Time::TIME_BASE.  We want two different
  *  versions of this class, dcpomatic::ContentTime and dcpomatic::DCPTime, and we want it to be impossible to
  *  convert implicitly between the two.  Hence there's this template hack.  I'm not
  *  sure if it's the best way to do it.
@@ -86,14 +87,13 @@ template <class S, class O>
 class Time
 {
 public:
-	Time ()
-		: _t (0)
-	{}
+	Time() = default;
 
-	typedef int64_t Type;
+	using Type = int64_t;
+	using big_int = boost::multiprecision::int128_t;
 
 	Time(Type n, Type d)
-		: _t (n * HZ / d)
+		: _t(big_int(big_int(n) * big_int(TIME_BASE) / big_int(d)).convert_to<int64_t>())
 	{}
 
 	/* Explicit conversion from type O */
@@ -168,19 +168,19 @@ public:
 	 *  @param r Sampling rate.
 	 */
 	Time<S, O> ceil (double r) const {
-		return Time<S, O> (llrint(HZ * frames_ceil(r) / r));
+		return Time<S, O>(llrint(frames_ceil(r) * TIME_BASE / r));
 	}
 
 	Time<S, O> floor (double r) const {
-		return Time<S, O> (llrint(HZ * frames_floor(r) / r));
+		return Time<S, O>(llrint(frames_floor(r) * TIME_BASE / r));
 	}
 
 	Time<S, O> round (double r) const {
-		return Time<S, O> (llrint(HZ * frames_round(r) / r));
+		return Time<S, O>(llrint(frames_round(r) * TIME_BASE / r));
 	}
 
 	double seconds () const {
-		return double (_t) / HZ;
+		return double (_t) / TIME_BASE;
 	}
 
 	Time<S, O> abs () const {
@@ -193,12 +193,12 @@ public:
 		   the calculation will round down before we get the chance
 		   to llrint().
 		*/
-		return llrint (_t * double(r) / HZ);
+		return llrint (_t * double(r) / TIME_BASE);
 	}
 
 	template <typename T>
 	int64_t frames_floor (T r) const {
-		return ::floor (_t * r / HZ);
+		return std::floor(_t * r / TIME_BASE);
 	}
 
 	template <typename T>
@@ -207,7 +207,7 @@ public:
 		   the calculation will round down before we get the chance
 		   to ceil().
 		*/
-		return ::ceil (_t * double(r) / HZ);
+		return std::ceil(_t * double(r) / TIME_BASE);
 	}
 
 	double proportion_of(Time<S, O> other) const {
@@ -267,13 +267,13 @@ public:
 	}
 
 	static Time<S, O> from_seconds (double s) {
-		return Time<S, O> (llrint (s * HZ));
+		return Time<S, O> (llrint (s * TIME_BASE));
 	}
 
 	template <class T>
 	static Time<S, O> from_frames (int64_t f, T r) {
 		DCPOMATIC_ASSERT (r > 0);
-		return Time<S, O> (f * HZ / r);
+		return Time<S, O>((double(f) / r) * TIME_BASE);
 	}
 
 	static Time<S, O> from_node(std::shared_ptr<const cxml::Node> node) {
@@ -281,25 +281,25 @@ public:
 			return {};
 		}
 
-		return Time<S, O>(dcp::raw_convert<Type>(node->content()), node->optional_number_attribute<Type>("timebase").get_value_or(96000));
+		return Time<S, O>(dcp::raw_convert<Type>(node->content()), node->optional_number_attribute<Type>("timebase").get_value_or(OLD_TIME_BASE));
 	}
 
 	xmlpp::Element* add_as_node(xmlpp::Element* parent, std::string name) const {
 		auto child = cxml::add_child(parent, name);
 		child->add_child_text(fmt::to_string(_t));
-		child->set_attribute("timebase", fmt::to_string(HZ));
+		child->set_attribute("timebase", fmt::to_string(TIME_BASE));
 		return child;
 	}
 
 	static Time<S, O> from_attributes(std::shared_ptr<const cxml::Node> node) {
 		auto time = number_attribute<Type>(node, "Time", "time");
-		auto timebase = node->optional_number_attribute<Type>("timebase").get_value_or(96000);
+		auto timebase = node->optional_number_attribute<Type>("timebase").get_value_or(OLD_TIME_BASE);
 		return Time<S, O>(time, timebase);
 	}
 
 	void add_as_attributes(xmlpp::Element* element) const {
 		element->set_attribute("time", fmt::to_string(_t));
-		element->set_attribute("timebase", fmt::to_string(HZ));
+		element->set_attribute("timebase", fmt::to_string(TIME_BASE));
 	}
 
 	static Time<S, O> delta () {
@@ -314,13 +314,15 @@ public:
 		return Time<S, O> (INT64_MAX);
 	}
 
-	static const int HZ = 96000;
+	static Type const TIME_BASE;
 
 private:
 	friend struct ::dcpomatic_time_ceil_test;
 	friend struct ::dcpomatic_time_floor_test;
 	friend class Time<O, S>;
 
+	static Type const OLD_TIME_BASE;
+
 	explicit Time (Type t)
 		: _t (t)
 	{}
@@ -329,10 +331,19 @@ private:
 		return _t;
 	}
 
-	Type _t;
+	Type _t = 0;
 };
 
 
+/* This is what old projects (before 2.18.0) assume */
+template <class S, class O>
+typename Time<S, O>::Type const Time<S, O>::OLD_TIME_BASE = 96000;
+
+/* See hacks/hz.py: this is divisible by a set of useful numbers */
+template <class S, class O>
+typename Time<S, O>::Type const Time<S, O>::TIME_BASE = 494236512000;
+
+
 class ContentTimeDifferentiator {};
 class DCPTimeDifferentiator {};
 
diff --git a/src/lib/fcpxml.cc b/src/lib/fcpxml.cc
index f6f3747e9..b7798b0b9 100644
--- a/src/lib/fcpxml.cc
+++ b/src/lib/fcpxml.cc
@@ -44,7 +44,7 @@ convert_time(string const& time)
 		throw FCPXMLError(fmt::format("Unexpected time format {}", time));
 	}
 
-	return dcpomatic::ContentTime{dcp::raw_convert<int64_t>(parts[0]) * dcpomatic::ContentTime::HZ / dcp::raw_convert<int64_t>(parts[1])};
+	return dcpomatic::ContentTime{dcp::raw_convert<int64_t>(parts[0]), dcp::raw_convert<int64_t>(parts[1])};
 }
 
 
diff --git a/src/lib/fcpxml_content.cc b/src/lib/fcpxml_content.cc
index e72ce2d7d..5164f8d1c 100644
--- a/src/lib/fcpxml_content.cc
+++ b/src/lib/fcpxml_content.cc
@@ -103,6 +103,6 @@ FCPXMLContent::as_xml(xmlpp::Element* element, bool with_paths, PathBehaviour pa
 		only_text()->as_xml(element);
 	}
 
-	cxml::add_child(element, "Length", fmt::to_string(_length.get()));
+	_length.add_as_node(element, "Length");
 }
 
diff --git a/src/lib/video_content.cc b/src/lib/video_content.cc
index 4b6680169..e4f944028 100644
--- a/src/lib/video_content.cc
+++ b/src/lib/video_content.cc
@@ -428,7 +428,7 @@ VideoContent::fade(shared_ptr<const Film> film, ContentTime time) const
 	auto const fade_in_time = ContentTime::from_frames(fade_in(), vfr);
 	/* time after the trimmed start of the content */
 	auto const time_after_start = time - trim_start;
-	if (fade_in_time.get() && time_after_start < fade_in_time) {
+	if (fade_in_time != ContentTime() && time_after_start < fade_in_time) {
 		return std::max(0.0, time_after_start.proportion_of(fade_in_time));
 	}
 
diff --git a/test/dcpomatic_time_test.cc b/test/dcpomatic_time_test.cc
index 4c5383d20..82d57a619 100644
--- a/test/dcpomatic_time_test.cc
+++ b/test/dcpomatic_time_test.cc
@@ -34,6 +34,10 @@ using std::cout;
 using namespace dcpomatic;
 
 
+// 24 hours
+int64_t constexpr long_project_seconds = 12 * 60 * 60;
+
+
 static DCPTime test_time(int t)
 {
 	return DCPTime(t, 96000);
@@ -57,6 +61,14 @@ BOOST_AUTO_TEST_CASE (dcpomatic_time_test)
 	}
 }
 
+
+BOOST_AUTO_TEST_CASE(dcpomatic_time_construction_test)
+{
+	/* Check that we can construct a large time using the old timebase */
+	BOOST_CHECK(DCPTime(long_project_seconds * 96000, 96000) == DCPTime(long_project_seconds * DCPTime::TIME_BASE, DCPTime::TIME_BASE));
+}
+
+
 BOOST_AUTO_TEST_CASE (dcpomatic_time_period_overlaps_test)
 {
 	/* Taking times as the start of a sampling interval
@@ -324,36 +336,55 @@ BOOST_AUTO_TEST_CASE (test_coalesce_with_overlapping_periods)
 /* Straightforward test of DCPTime::ceil */
 BOOST_AUTO_TEST_CASE (dcpomatic_time_ceil_test)
 {
-	BOOST_CHECK(test_time(0).ceil(DCPTime::HZ / 2) == test_time(0));
-	BOOST_CHECK(test_time(1).ceil(DCPTime::HZ / 2) == test_time(2));
-	BOOST_CHECK(test_time(2).ceil(DCPTime::HZ / 2) == test_time(2));
-	BOOST_CHECK(test_time(3).ceil(DCPTime::HZ / 2) == test_time(4));
+	BOOST_CHECK(DCPTime(0, DCPTime::TIME_BASE).ceil(DCPTime::TIME_BASE / 2) == DCPTime(0, DCPTime::TIME_BASE));
+	BOOST_CHECK(DCPTime(1, DCPTime::TIME_BASE).ceil(DCPTime::TIME_BASE / 2) == DCPTime(2, DCPTime::TIME_BASE));
+	BOOST_CHECK(DCPTime(2, DCPTime::TIME_BASE).ceil(DCPTime::TIME_BASE / 2) == DCPTime(2, DCPTime::TIME_BASE));
+	BOOST_CHECK(DCPTime(3, DCPTime::TIME_BASE).ceil(DCPTime::TIME_BASE / 2) == DCPTime(4, DCPTime::TIME_BASE));
 
-	BOOST_CHECK(test_time(0).ceil(DCPTime::HZ / 42) == test_time(0));
-	BOOST_CHECK(test_time(1).ceil(DCPTime::HZ / 42) == test_time(42));
-	BOOST_CHECK(test_time(42).ceil(DCPTime::HZ / 42) == test_time(42));
-	BOOST_CHECK(test_time(43).ceil(DCPTime::HZ / 42) == test_time(84));
+	BOOST_CHECK(DCPTime(0, DCPTime::TIME_BASE).ceil(DCPTime::TIME_BASE / 42) == DCPTime(0, DCPTime::TIME_BASE));
+	BOOST_CHECK(DCPTime(1, DCPTime::TIME_BASE).ceil(DCPTime::TIME_BASE / 42) == DCPTime(42, DCPTime::TIME_BASE));
+	BOOST_CHECK(DCPTime(42, DCPTime::TIME_BASE).ceil(DCPTime::TIME_BASE / 42) == DCPTime(42, DCPTime::TIME_BASE));
+	BOOST_CHECK(DCPTime(43, DCPTime::TIME_BASE).ceil(DCPTime::TIME_BASE / 42) == DCPTime(84, DCPTime::TIME_BASE));
 
 	/* Check that rounding up to non-integer frame rates works */
-	BOOST_CHECK(test_time(45312).ceil(29.976) == test_time(48038));
+	BOOST_CHECK(test_time(45312).ceil(29.976) == DCPTime(247316108887, DCPTime::TIME_BASE));
 
 	/* Check another tricky case that used to fail */
-	BOOST_CHECK(test_time(212256039).ceil(23.976) == test_time(212256256));
+	BOOST_CHECK(DCPTime(1092757128815583LL, DCPTime::TIME_BASE).ceil(23.976) == DCPTime(1092758247315315, DCPTime::TIME_BASE));
 }
 
 /* Straightforward test of DCPTime::floor */
 BOOST_AUTO_TEST_CASE (dcpomatic_time_floor_test)
 {
-	BOOST_CHECK(test_time(0).floor(DCPTime::HZ / 2) == test_time(0));
-	BOOST_CHECK(test_time(1).floor(DCPTime::HZ / 2) == test_time(0));
-	BOOST_CHECK(test_time(2).floor(DCPTime::HZ / 2) == test_time(2));
-	BOOST_CHECK(test_time(3).floor(DCPTime::HZ / 2) == test_time(2));
+	BOOST_CHECK(test_time(0).floor(DCPTime::TIME_BASE / 2) == test_time(0));
+	BOOST_CHECK(test_time(1).floor(DCPTime::TIME_BASE / 2) == test_time(0));
+	BOOST_CHECK(test_time(2).floor(DCPTime::TIME_BASE / 2) == test_time(2));
+	BOOST_CHECK(test_time(3).floor(DCPTime::TIME_BASE / 2) == test_time(2));
 
-	BOOST_CHECK(test_time(0).floor(DCPTime::HZ / 42) == test_time(0));
-	BOOST_CHECK(test_time(1).floor(DCPTime::HZ / 42) == test_time(0));
-	BOOST_CHECK(test_time(42).floor(DCPTime::HZ / 42.0) == test_time(42));
-	BOOST_CHECK(test_time(43).floor(DCPTime::HZ / 42.0) == test_time(42));
+	BOOST_CHECK(test_time(0).floor(DCPTime::TIME_BASE / 42) == test_time(0));
+	BOOST_CHECK(test_time(1).floor(DCPTime::TIME_BASE / 42) == test_time(0));
+	BOOST_CHECK(test_time(42).floor(DCPTime::TIME_BASE / 42.0) == test_time(42));
+	BOOST_CHECK(test_time(43).floor(DCPTime::TIME_BASE / 42.0) == test_time(42));
 
 	/* Check that rounding down to non-integer frame rates works */
 	BOOST_CHECK(test_time(45312).floor(29.976) == test_time(44836));
 }
+
+
+BOOST_AUTO_TEST_CASE(dcpomatic_time_factor_test)
+{
+	for (int i = 2; i <= 30; ++i) {
+		BOOST_CHECK((dcpomatic::DCPTime::TIME_BASE % i) == 0);
+	}
+
+	BOOST_CHECK((dcpomatic::DCPTime::TIME_BASE % 36) == 0);
+	BOOST_CHECK((dcpomatic::DCPTime::TIME_BASE % 48) == 0);
+	BOOST_CHECK((dcpomatic::DCPTime::TIME_BASE % 60) == 0);
+}
+
+
+BOOST_AUTO_TEST_CASE(dcpomatic_time_frames_floor_test)
+{
+	BOOST_CHECK_EQUAL(dcpomatic::DCPTime(195061250884500LL, 69872686884000LL).frames_floor(48000), 134000);
+}
+