Store a numerator and denominator in agi::vfr::Framerate rather than a fps. Fixes minor rounding issues with 24000/1001 and 30000/1001 frame rates.

Originally committed to SVN as r6115.

aegisub/libaegisub/common/vfr.cpp

@@ -32,6 +32,7 @@
 #include "libaegisub/charset.h"
 #include "libaegisub/io.h"
 #include "libaegisub/line_iterator.h"
+#include "libaegisub/scoped_ptr.h"
 
 namespace std {
 	template<> void swap(agi::vfr::Framerate &lft, agi::vfr::Framerate &rgt) {
@@ -39,6 +40,8 @@ namespace std {
 	}
 }
 
+static const int64_t default_denominator = 1000000000;
+
 namespace agi {
 namespace vfr {
 
@@ -148,9 +151,9 @@ static void v1_fill_range_gaps(std::list<TimecodeRange> &ranges, double fps) {
 /// @param      file      Iterator of lines in the file
 /// @param      line      Header of file with assumed fps
 /// @param[out] timecodes Vector filled with frame start times
-/// @param[out] time      Unrounded time of the last frame
+/// @param[out] last      Unrounded time of the last frame
-/// @return Assumed fps
+/// @return Assumed fps times one million
-static double v1_parse(line_iterator<std::string> file, std::string line, std::vector<int> &timecodes, double &time) {
+static int64_t v1_parse(line_iterator<std::string> file, std::string line, std::vector<int> &timecodes, int64_t &last) {
 	using namespace std;
 	double fps = atof(line.substr(7).c_str());
 	if (fps <= 0.) throw BadFPS("Assumed FPS must be greater than zero");
@@ -164,7 +167,7 @@ static double v1_parse(line_iterator<std::string> file, std::string line, std::v
 	v1_fill_range_gaps(ranges, fps);
 	timecodes.reserve(ranges.back().end);
 
-	time = 0.;
+	double time = 0.;
 	for (list<TimecodeRange>::iterator cur = ranges.begin(); cur != ranges.end(); ++cur) {
 		for (int frame = cur->start; frame <= cur->end; frame++) {
 			timecodes.push_back(int(time + .5));
@@ -172,35 +175,59 @@ static double v1_parse(line_iterator<std::string> file, std::string line, std::v
 		}
 	}
 	timecodes.push_back(int(time + .5));
-	return fps;
+	last = int64_t(time * fps * default_denominator);
+	return int64_t(fps * default_denominator);
 }
 
 Framerate::Framerate(Framerate const& that)
-: fps(that.fps)
+: numerator(that.numerator)
+, denominator(that.denominator)
 , last(that.last)
 , timecodes(that.timecodes)
 {
 }
 
-Framerate::Framerate(double fps) : fps(fps), last(0.) {
+Framerate::Framerate(double fps)
+: denominator(default_denominator)
+, numerator(int64_t(fps * denominator))
+, last(0)
+{
 	if (fps < 0.) throw BadFPS("FPS must be greater than zero");
 	if (fps > 1000.) throw BadFPS("FPS must not be greater than 1000");
+	timecodes.push_back(0);
+}
+
+Framerate::Framerate(int64_t numerator, int64_t denominator)
+: denominator(denominator)
+, numerator(numerator)
+, last(0)
+{
+	if (numerator <= 0 || denominator <= 0)
+		throw BadFPS("Numerator and denominator must both be greater than zero");
+	if (numerator / denominator > 1000) throw BadFPS("FPS must not be greater than 1000");
+	timecodes.push_back(0);
+}
+
+void Framerate::SetFromTimecodes() {
+	validate_timecodes(timecodes);
+	normalize_timecodes(timecodes);
+	denominator = default_denominator;
+	numerator = (timecodes.size() - 1) * denominator * 1000 / timecodes.back();
+	last = (timecodes.size() - 1) * denominator * 1000;
 }
 
 Framerate::Framerate(std::vector<int> const& timecodes)
 : timecodes(timecodes)
 {
-	validate_timecodes(timecodes);
+	SetFromTimecodes();
-	normalize_timecodes(this->timecodes);
-	fps = (timecodes.size() - 1) * 1000. / timecodes.back();
-	last = timecodes.back();
 }
 
 Framerate::~Framerate() {
 }
 
 void Framerate::swap(Framerate &right) throw() {
-	std::swap(fps, right.fps);
+	std::swap(numerator, right.numerator);
+	std::swap(denominator, right.denominator);
 	std::swap(last, right.last);
 	std::swap(timecodes, right.timecodes);
 }
@@ -213,24 +240,24 @@ Framerate &Framerate::operator=(double fps) {
 	return *this = Framerate(fps);
 }
 
-Framerate::Framerate(std::string const& filename) : fps(0.) {
+Framerate::Framerate(std::string const& filename)
+: denominator(default_denominator)
+, numerator(0)
+{
 	using namespace std;
-	auto_ptr<ifstream> file(agi::io::Open(filename));
+	scoped_ptr<ifstream> file(agi::io::Open(filename));
 	string encoding = agi::charset::Detect(filename);
 	string line = *line_iterator<string>(*file, encoding);
 	if (line == "# timecode format v2") {
 		copy(line_iterator<int>(*file, encoding), line_iterator<int>(), back_inserter(timecodes));
-		validate_timecodes(timecodes);
+		SetFromTimecodes();
-		normalize_timecodes(timecodes);
-		fps = (timecodes.size() - 1) * 1000. / timecodes.back();
-		last = timecodes.back();
 		return;
 	}
 	if (line == "# timecode format v1" || line.substr(0, 7) == "Assume ") {
 		if (line[0] == '#') {
 			line = *line_iterator<string>(*file, encoding);
 		}
-		fps = v1_parse(line_iterator<string>(*file, encoding), line, timecodes, last);
+		numerator = v1_parse(line_iterator<string>(*file, encoding), line, timecodes, last);
 		return;
 	}
 
@@ -248,10 +275,6 @@ void Framerate::Save(std::string const& filename, int length) const {
 	}
 }
 
-static int round(double value) {
-	return int(value + .5);
-}
-
 int Framerate::FrameAtTime(int ms, Time type) const {
 	// With X ms per frame, this should return 0 for:
 	// EXACT: [0, X - 1]
@@ -273,17 +296,13 @@ int Framerate::FrameAtTime(int ms, Time type) const {
 		return FrameAtTime(ms - 1);
 	}
 
-	if (timecodes.empty()) {
+	if (ms < 0)
-		return (int)floor(ms * fps / 1000.);
+		return int((ms * numerator / denominator - 999) / 1000);
-	}
-	if (ms < 0) {
-		return (int)floor(ms * fps / 1000.);
-	}
-	if (ms > timecodes.back()) {
-		return round((ms - timecodes.back()) * fps / 1000.) + (int)timecodes.size() - 1;
-	}
 
-	return (int)std::distance(std::lower_bound(timecodes.rbegin(), timecodes.rend(), ms, std::greater<int>()), timecodes.rend()) - 1;
+	if (ms > timecodes.back())
+		return int((ms * numerator - last + denominator - 1) / denominator / 1000) + (int)timecodes.size() - 1;
+
+	return (int)distance(lower_bound(timecodes.rbegin(), timecodes.rend(), ms, std::greater<int>()), timecodes.rend()) - 1;
 }
 
 int Framerate::TimeAtFrame(int frame, Time type) const {
@@ -293,22 +312,21 @@ int Framerate::TimeAtFrame(int frame, Time type) const {
 		// + 1 as these need to round up for the case of two frames 1 ms apart
 		return prev + (cur - prev + 1) / 2;
 	}
+
 	if (type == END) {
 		int cur = TimeAtFrame(frame);
 		int next = TimeAtFrame(frame + 1);
 		return cur + (next - cur + 1) / 2;
 	}
 
-	if (timecodes.empty()) {
+	if (frame < 0)
-		return (int)ceil(frame / fps * 1000.);
+		return (int)(frame * denominator * 1000 / numerator);
+
+	if (frame >= (signed)timecodes.size()) {
+		int64_t frames_past_end = frame - (int)timecodes.size() + 1;
+		return int((frames_past_end * 1000 * denominator + last + numerator / 2) / numerator);
 	}
 
-	if (frame < 0) {
-		return (int)ceil(frame / fps * 1000.);
-	}
-	if (frame >= (signed)timecodes.size()) {
-		return round((frame - timecodes.size() + 1) * 1000. / fps + last);
-	}
 	return timecodes[frame];
 }
 

aegisub/libaegisub/include/libaegisub/vfr.h

@@ -23,6 +23,8 @@
 #ifndef LAGI_PRE
 #include <string>
 #include <vector>
+
+#include <stdint.h>
 #endif
 
 #include <libaegisub/exception.h>
@@ -62,16 +64,31 @@ DEFINE_SIMPLE_EXCEPTION_NOINNER(UnorderedTimecodes, Error, "vfr/timecodes/order"
 /// @brief Class for managing everything related to converting frames to times
 ///        or vice versa
 class Framerate {
-	/// Average FPS for v2, assumed FPS for v1, fps for CFR
+	/// Denominator of the FPS
-	double fps;
+	///
-	/// Unrounded time of the last frame in a v1 override range. Needed to
+	/// For v1 VFR, the assumed FPS is used, for v2 the average FPS
-	/// match mkvmerge's rounding
+	int64_t denominator;
-	double last;
+	/// Numerator of the FPS
+	///
+	/// For v1 VFR, the assumed FPS is used, for v2 the average FPS
+	int64_t numerator;
+
+	/// Unrounded frame-seconds of the final frame in timecodes. For CFR and v2,
+	/// this is simply frame count * denominator, but for v1 it's the
+	/// "unrounded" frame count, since override ranges generally don't exactly
+	/// cover timebase-unit ranges of time. This is needed to match mkvmerge's
+	/// rounding past the end of the final override range.
+	int64_t last;
+
 	/// Start time in milliseconds of each frame
 	std::vector<int> timecodes;
+
+	/// Set FPS properties from the timecodes vector
+	void SetFromTimecodes();
 public:
 	/// Copy constructor
 	Framerate(Framerate const&);
+
 	/// @brief VFR from timecodes file
 	/// @param filename File with v1 or v2 timecodes
 	///
@@ -80,13 +97,23 @@ public:
 	/// mkvmerge-style rounding is applied, while setting a constant frame rate
 	/// uses truncation.
 	Framerate(std::string const& filename);
+
 	/// @brief CFR constructor
 	/// @param fps Frames per second or 0 for unloaded
 	Framerate(double fps = 0.);
+
+	/// @brief CFR constructor with rational timebase
+	/// @param numerator Timebase numerator
+	/// @param denominator Timebase denominator
+	Framerate(int64_t numerator, int64_t denominator);
+
 	/// @brief VFR from frame times
 	/// @param timecodes Vector of frame start times in milliseconds
 	Framerate(std::vector<int> const& timecodes);
+
+	/// Destructor
 	~Framerate();
+
 	/// Atomic assignment operator
 	Framerate &operator=(Framerate);
 	/// Atomic CFR assignment operator
@@ -130,9 +157,9 @@ public:
 	/// be otherwise sensible.
 	void Save(std::string const& file, int length = -1) const;
 
-	bool IsVFR() const {return !timecodes.empty(); }
+	bool IsVFR() const {return timecodes.size() > 1; }
-	bool IsLoaded() const { return !timecodes.empty() || fps; };
+	bool IsLoaded() const { return numerator > 0; };
-	double FPS() const { return fps; }
+	double FPS() const { return double(numerator) / denominator; }
 };
 
 	} // namespace vfr

aegisub/tests/libaegisub_vfr.cpp

@@ -20,6 +20,7 @@
 
 #include <libaegisub/vfr.h>
 
+#include <climits>
 #include <fstream>
 #include <iterator>
 
@@ -334,7 +335,7 @@ TEST(lagi_vfr, save_vfr_len) {
 TEST(lagi_vfr, load_v2) {
 	Framerate fps;
 	ASSERT_NO_THROW(fps = Framerate("data/vfr/in/v2_1fps.txt"));
-	for (int i = 0; i < 9; i++) {
+	for (int i = 0; i < 30; i++) {
 		EXPECT_EQ(i * 1000, fps.TimeAtFrame(i));
 	}
 }
@@ -342,7 +343,7 @@ TEST(lagi_vfr, load_v2) {
 TEST(lagi_vfr, load_v2_comments) {
 	Framerate fps;
 	ASSERT_NO_THROW(fps = Framerate("data/vfr/in/v2_comments.txt"));
-	for (int i = 0; i < 9; i++) {
+	for (int i = 0; i < 30; i++) {
 		EXPECT_EQ(i * 1000, fps.TimeAtFrame(i));
 	}
 }
@@ -350,7 +351,7 @@ TEST(lagi_vfr, load_v2_comments) {
 TEST(lagi_vfr, load_v2_number_in_comment) {
 	Framerate fps;
 	ASSERT_NO_THROW(fps = Framerate("data/vfr/in/v2_number_in_comment.txt"));
-	for (int i = 0; i < 9; i++) {
+	for (int i = 0; i < 30; i++) {
 		EXPECT_EQ(i * 1000, fps.TimeAtFrame(i));
 	}
 }
@@ -394,3 +395,28 @@ TEST(lagi_vfr, nonzero_start_time) {
 	EXPECT_EQ(50, fps.TimeAtFrame(3, EXACT));
 	EXPECT_EQ(60, fps.TimeAtFrame(4, EXACT));
 }
+
+TEST(lagi_vfr, rational_timebase) {
+	Framerate fps;
+
+	ASSERT_NO_THROW(fps = Framerate(30000, 1001));
+	for (int i = 0; i < 100000; ++i) {
+		EXPECT_EQ(i * 1001, fps.TimeAtFrame(i * 30, EXACT));
+		EXPECT_EQ(i * 30, fps.FrameAtTime(i * 1001, EXACT));
+	}
+
+	ASSERT_NO_THROW(fps = Framerate(24000, 1001));
+	for (int i = 0; i < 100000; ++i) {
+		EXPECT_EQ(i * 1001, fps.TimeAtFrame(i * 24, EXACT));
+		EXPECT_EQ(i * 24, fps.FrameAtTime(i * 1001, EXACT));
+	}
+}
+
+TEST(lagi_vfr, no_intermediate_overflow) {
+	Framerate fps;
+
+	ASSERT_NO_THROW(fps = Framerate(1.0));
+	int last_frame = INT_MAX / 1000;
+	EXPECT_EQ(last_frame * 1000, fps.TimeAtFrame(last_frame, EXACT));
+	EXPECT_EQ(last_frame, fps.FrameAtTime(last_frame * 1000, EXACT));
+}