Give DCPFrameRate a constructor. Add repeat member and cleverer dcp frame rate calculations.

8 files changed, 88 insertions, 43 deletions

diff --git a/src/lib/check_hashes_job.cc b/src/lib/check_hashes_job.cc
index 701584c74..6f6591396 100644
--- a/src/lib/check_hashes_job.cc
+++ b/src/lib/check_hashes_job.cc
@@ -59,9 +59,11 @@ CheckHashesJob::run ()
 	}
 	
 	SourceFrame const N = _film->dcp_trim_start() + _film->dcp_length().get();
-	DCPFrameRate const dfr = dcp_frame_rate (_film->frames_per_second ());
+	DCPFrameRate const dfr (_film->frames_per_second ());
+
+	int const inc = dfr.skip ? 2 : 1;
 	
-	for (SourceFrame i = _film->dcp_trim_start(); i < N; i += dfr.skip) {
+	for (SourceFrame i = _film->dcp_trim_start(); i < N; i += inc) {
 		string const j2k_file = _encode_opt->frame_out_path (i, false);
 		string const hash_file = _encode_opt->hash_out_path (i, false);
 
diff --git a/src/lib/dcp_video_frame.cc b/src/lib/dcp_video_frame.cc
index c6b29ba41..f6fb8fe2e 100644
--- a/src/lib/dcp_video_frame.cc
+++ b/src/lib/dcp_video_frame.cc
@@ -66,8 +66,8 @@ using boost::shared_ptr;
  *  @param out Required size of output, in pixels (including any padding).
  *  @param s Scaler to use.
  *  @param p Number of pixels of padding either side of the image.
- *  @param f Index of the frame within the Film.
- *  @param fps Frames per second of the Film.
+ *  @param f Index of the frame within the Film's source.
+ *  @param fps Frames per second of the Film's source.
  *  @param pp FFmpeg post-processing string to use.
  *  @param clut Colour look-up table to use (see Config::colour_lut_index ())
  *  @param bw J2K bandwidth to use (see Config::j2k_bandwidth ())
@@ -86,7 +86,7 @@ DCPVideoFrame::DCPVideoFrame (
 	, _subtitle_scale (subtitle_scale)
 	, _scaler (s)
 	, _frame (f)
-	, _frames_per_second (dcp_frame_rate(fps).frames_per_second)
+	, _frames_per_second (DCPFrameRate(fps).frames_per_second)
 	, _post_process (pp)
 	, _colour_lut (clut)
 	, _j2k_bandwidth (bw)
diff --git a/src/lib/encoder.cc b/src/lib/encoder.cc
index efedfcfef..afa636150 100644
--- a/src/lib/encoder.cc
+++ b/src/lib/encoder.cc
@@ -279,7 +279,7 @@ Encoder::frame_skipped ()
 void
 Encoder::process_video (shared_ptr<Image> image, bool same, boost::shared_ptr<Subtitle> sub)
 {
-	if (_opt->video_skip != 0 && (_video_frame % _opt->video_skip) != 0) {
+	if (_opt->video_skip && (_video_frame % 2)) {
 		++_video_frame;
 		return;
 	}
diff --git a/src/lib/film.cc b/src/lib/film.cc
index f0441c9e0..17e14e544 100644
--- a/src/lib/film.cc
+++ b/src/lib/film.cc
@@ -294,25 +294,26 @@ Film::make_dcp (bool transcode)
 	if (dcp_length ()) {
 		oe->video_range = make_pair (dcp_trim_start(), dcp_trim_start() + dcp_length().get());
 		if (audio_stream()) {
+			DCPFrameRate dfr (frames_per_second ());
 			oe->audio_range = make_pair (
 
 				video_frames_to_audio_frames (
 					oe->video_range.get().first,
 					dcp_audio_sample_rate (audio_stream()->sample_rate()),
-					dcp_frame_rate (frames_per_second()).frames_per_second
+					dfr.frames_per_second
 					),
 				
 				video_frames_to_audio_frames (
 					oe->video_range.get().second,
 					dcp_audio_sample_rate (audio_stream()->sample_rate()),
-					dcp_frame_rate (frames_per_second()).frames_per_second
+					dfr.frames_per_second
 					)
 				);
 		}
 			
 	}
 	
-	oe->video_skip = dcp_frame_rate (frames_per_second()).skip;
+	oe->video_skip = DCPFrameRate (frames_per_second()).skip;
 
 	shared_ptr<DecodeOptions> od (new DecodeOptions);
 	od->decode_subtitles = with_subtitles ();
@@ -704,13 +705,15 @@ Film::target_audio_sample_rate () const
 	/* Resample to a DCI-approved sample rate */
 	double t = dcp_audio_sample_rate (audio_stream()->sample_rate());
 
-	DCPFrameRate dfr = dcp_frame_rate (frames_per_second ());
+	DCPFrameRate dfr (frames_per_second ());
 
 	/* Compensate for the fact that video will be rounded to the
 	   nearest integer number of frames per second.
 	*/
+
+	int const mult = dfr.skip ? 2 : 1;
 	if (dfr.run_fast) {
-		t *= _frames_per_second * dfr.skip / dfr.frames_per_second;
+		t *= _frames_per_second * mult / dfr.frames_per_second;
 	}
 
 	return rint (t);
diff --git a/src/lib/make_dcp_job.cc b/src/lib/make_dcp_job.cc
index b9f0cacf3..67a2d8b13 100644
--- a/src/lib/make_dcp_job.cc
+++ b/src/lib/make_dcp_job.cc
@@ -61,7 +61,9 @@ MakeDCPJob::name () const
 string
 MakeDCPJob::j2c_path (int f, int offset) const
 {
-	SourceFrame const s = ((f + offset) * dcp_frame_rate(_film->frames_per_second()).skip) + _film->dcp_trim_start();
+	DCPFrameRate dfr (_film->frames_per_second());
+	int const mult = dfr.skip ? 2 : 1;
+	SourceFrame const s = ((f + offset) * mult) + _film->dcp_trim_start();
 	return _opt->frame_out_path (s, false);
 }
 
@@ -85,13 +87,16 @@ MakeDCPJob::run ()
 	/* Remove any old DCP */
 	boost::filesystem::remove_all (dcp_path);
 
-	DCPFrameRate const dfr = dcp_frame_rate (_film->frames_per_second ());
+	DCPFrameRate const dfr (_film->frames_per_second ());
 
 	int frames = 0;
 	switch (_film->content_type ()) {
 	case VIDEO:
 		/* Source frames -> DCP frames */
-		frames = _film->dcp_length().get() / dfr.skip;
+		frames = _film->dcp_length().get();
+		if (dfr.skip) {
+			frames /= 2;
+		}
 		break;
 	case STILL:
 		frames = _film->still_duration() * 24;
diff --git a/src/lib/options.h b/src/lib/options.h
index 55b066a2d..d1fc1d54e 100644
--- a/src/lib/options.h
+++ b/src/lib/options.h
@@ -39,7 +39,7 @@ public:
 
 	EncodeOptions (std::string f, std::string e, std::string m)
 		: padding (0)
-		, video_skip (0)
+		, video_skip (false)
 		, _frame_out_path (f)
 		, _frame_out_extension (e)
 		, _multichannel_audio_out_path (m)
@@ -98,10 +98,8 @@ public:
 	/** Range of audio frames to decode (in the DCP's sampling rate) */
 	boost::optional<std::pair<int64_t, int64_t> > audio_range;
 	
-	/** Skip frames such that we don't decode any frame where (index % decode_video_skip) != 0; e.g.
-	 *  1 for every frame, 2 for every other frame, etc.
-	 */
-	SourceFrame video_skip; 
+	/** true to skip every other frame */
+	SourceFrame video_skip;
 
 private:
 	/** Path of the directory to write video frames to */
diff --git a/src/lib/util.cc b/src/lib/util.cc
index 0fced638c..4228ce6cf 100644
--- a/src/lib/util.cc
+++ b/src/lib/util.cc
@@ -330,25 +330,55 @@ md5_digest (string file)
 	return s.str ();
 }
 
-/** @param fps Arbitrary frames-per-second value.
- *  @return DCPFrameRate for this frames-per-second.
- */
-DCPFrameRate
-dcp_frame_rate (float fps)
-{
-	DCPFrameRate dfr;
-
-	dfr.run_fast = (fps != rint (fps));
-	dfr.frames_per_second = rint (fps);
-	dfr.skip = 1;
-
-	/* XXX: somewhat arbitrary */
-	if (fps == 50) {
-		dfr.frames_per_second = 25;
-		dfr.skip = 2;
+static bool about_equal (float a, float b)
+{
+	/* A film of F seconds at f FPS will be Ff frames;
+	   Consider some delta FPS d, so if we run the same
+	   film at (f + d) FPS it will last F(f + d) seconds.
+
+	   Hence the difference in length over the length of the film will
+	   be F(f + d) - Ff frames
+	    = Ff + Fd - Ff frames
+	    = Fd frames
+	    = Fd/f seconds
+ 
+	   So if we accept a difference of 1 frame, ie 1/f seconds, we can
+	   say that
+
+	   1/f = Fd/f
+	ie 1 = Fd
+	ie d = 1/F
+ 
+	   So for a 3hr film, ie F = 3 * 60 * 60 = 10800, the acceptable
+	   FPS error is 1/F ~= 0.0001 ~= 10-e4
+	*/
+
+	return (fabs (a - b) < 1e-4);
+}
+
+/** @param fps Arbitrary source frames-per-second value */
+DCPFrameRate::DCPFrameRate (float fps)
+	: frames_per_second (rint (fps))
+	, skip (false)
+	, repeat (false)
+	, run_fast (false)
+{
+	if (about_equal (fps, 50)) {
+		/* XXX: not sure about this; just run at 50?
+		   Ring Peter Jackson.
+		*/
+		frames_per_second = 25;
+		skip = true;
+	} else if (fps >= (27.5 / 2) && fps <= (32.5 / 2)) {
+		frames_per_second = 30;
+		repeat = true;
+	} else if (fps >= (24.5 / 2) && fps <= (27.5 / 2)) {
+		frames_per_second = 25;
+		repeat = true;
+	} else if (fps >= (20 / 2) && fps <= (24.5 / 2)) {
+		frames_per_second = 24;
+		repeat = true;
 	}
-
-	return dfr;
 }
 
 /** @param An arbitrary sampling rate.
diff --git a/src/lib/util.h b/src/lib/util.h
index 024c40fb5..50e7410d6 100644
--- a/src/lib/util.h
+++ b/src/lib/util.h
@@ -61,14 +61,22 @@ typedef int SourceFrame;
 
 struct DCPFrameRate
 {
+	DCPFrameRate (float);
+	
 	/** frames per second for the DCP */
 	int frames_per_second;
-	/** Skip every `skip' frames.  e.g. if this is 1, we skip nothing;
-	 *  if it's 2, we skip every other frame.
-	 */
-	int skip;
+	/** true to skip every other frame */
+	bool skip;
+	/** true to repeat every frame once */
+	bool repeat;
 	/** true if this DCP will run its video faster than the source
-	 *  (e.g. if the source is 29.97fps and we will run the DCP at 30fps)
+	 *  without taking into account `skip' and `repeat'.
+	 *  (i.e. run_fast will be true if
+	 *          source is 29.97fps, DCP is 30fps
+	 *          source is 14.50fps, DCP is 30fps
+	 *  but not if
+	 *          source is 15.00fps, DCP is 30fps
+	 *          source is 12.50fps, DCP is 25fps)
 	 */
 	bool run_fast;
 };
@@ -183,7 +191,6 @@ struct Rect
 
 extern std::string crop_string (Position, Size);
 extern int dcp_audio_sample_rate (int);
-extern DCPFrameRate dcp_frame_rate (float);
 extern int dcp_audio_channels (int);
 extern std::string colour_lut_index_to_name (int index);
 extern int stride_round_up (int, int const *, int);