Various fixes to FFmpeg decoder, including a couple of tests.

7 files changed, 85 insertions, 37 deletions

diff --git a/src/lib/decoder.h b/src/lib/decoder.h
index df3ac4f39..38556c818 100644
--- a/src/lib/decoder.h
+++ b/src/lib/decoder.h
@@ -42,10 +42,12 @@ public:
 	virtual ~Decoder () {}
 
 protected:	
-	/** Seek so that the next peek() will yield the next thing
+	/** Seek so that the next pass() will yield the next thing
 	 *  (video/sound frame, subtitle etc.) at or after the requested
 	 *  time.  Pass accurate = true to try harder to get close to
-	 *  the request.
+	 *  the request.  Note that seeking to time t may mean that
+	 *  the next pass() yields, for example, audio at time t and then
+	 *  video before it.
 	 */
 	virtual void seek (ContentTime time, bool accurate) = 0;
 	virtual bool pass () = 0;
diff --git a/src/lib/ffmpeg_decoder.cc b/src/lib/ffmpeg_decoder.cc
index c9efd80fc..0a4624569 100644
--- a/src/lib/ffmpeg_decoder.cc
+++ b/src/lib/ffmpeg_decoder.cc
@@ -1,5 +1,5 @@
 /*
-    Copyright (C) 2012 Carl Hetherington <cth@carlh.net>
+    Copyright (C) 2012-2014 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
@@ -313,9 +313,9 @@ FFmpegDecoder::minimal_run (boost::function<bool (optional<ContentTime>, optiona
 
 			avcodec_get_frame_defaults (_frame);
 			
-			int finished = 0;
-			r = avcodec_decode_video2 (video_codec_context(), _frame, &finished, &_packet);
-			if (r >= 0 && finished) {
+			int got_picture = 0;
+			r = avcodec_decode_video2 (video_codec_context(), _frame, &got_picture, &_packet);
+			if (r >= 0 && got_picture) {
 				last_video = ContentTime::from_seconds (av_frame_get_best_effort_timestamp (_frame) * time_base) + _pts_offset;
 			}
 
@@ -323,9 +323,9 @@ FFmpegDecoder::minimal_run (boost::function<bool (optional<ContentTime>, optiona
 			AVPacket copy_packet = _packet;
 			while (copy_packet.size > 0) {
 
-				int finished;
-				r = avcodec_decode_audio4 (audio_codec_context(), _frame, &finished, &_packet);
-				if (r >= 0 && finished) {
+				int got_frame;
+				r = avcodec_decode_audio4 (audio_codec_context(), _frame, &got_frame, &_packet);
+				if (r >= 0 && got_frame) {
 					last_audio = ContentTime::from_seconds (av_frame_get_best_effort_timestamp (_frame) * time_base) + _pts_offset;
 				}
 					
@@ -387,12 +387,12 @@ FFmpegDecoder::seek (ContentTime time, bool accurate)
 	VideoDecoder::seek (time, accurate);
 	AudioDecoder::seek (time, accurate);
 	
-	/* If we are doing an accurate seek, our initial shot will be 200ms (200 being
+	/* If we are doing an accurate seek, our initial shot will be 2s (2 being
 	   a number plucked from the air) earlier than we want to end up.  The loop below
 	   will hopefully then step through to where we want to be.
 	*/
 
-	ContentTime pre_roll = accurate ? ContentTime::from_seconds (0.2) : ContentTime (0);
+	ContentTime pre_roll = accurate ? ContentTime::from_seconds (2) : ContentTime (0);
 	ContentTime initial_seek = time - pre_roll;
 	if (initial_seek < ContentTime (0)) {
 		initial_seek = ContentTime (0);
@@ -551,7 +551,9 @@ FFmpegDecoder::decode_subtitle_packet ()
 	AVSubtitleRect const * rect = sub.rects[0];
 
 	if (rect->type != SUBTITLE_BITMAP) {
-		throw DecodeError (_("non-bitmap subtitles not yet supported"));
+		/* XXX */
+		// throw DecodeError (_("non-bitmap subtitles not yet supported"));
+		return;
 	}
 
 	/* Note RGBA is expressed little-endian, so the first byte in the word is R, second
diff --git a/src/lib/ffmpeg_examiner.cc b/src/lib/ffmpeg_examiner.cc
index 6daba4b40..72db9bce1 100644
--- a/src/lib/ffmpeg_examiner.cc
+++ b/src/lib/ffmpeg_examiner.cc
@@ -109,7 +109,7 @@ FFmpegExaminer::frame_time (AVStream* s) const
 	
 	int64_t const bet = av_frame_get_best_effort_timestamp (_frame);
 	if (bet != AV_NOPTS_VALUE) {
-		t = ContentTime (bet * av_q2d (s->time_base));
+		t = ContentTime::from_seconds (bet * av_q2d (s->time_base));
 	}
 
 	return t;
@@ -118,13 +118,11 @@ FFmpegExaminer::frame_time (AVStream* s) const
 float
 FFmpegExaminer::video_frame_rate () const
 {
-	AVStream* s = _format_context->streams[_video_stream];
-
-	if (s->avg_frame_rate.num && s->avg_frame_rate.den) {
-		return av_q2d (s->avg_frame_rate);
-	}
-
-	return av_q2d (s->r_frame_rate);
+	/* This use of r_frame_rate is debateable; there's a few different
+	 * frame rates in the format context, but this one seems to be the most
+	 * reliable.
+	 */
+	return av_q2d (av_stream_get_r_frame_rate (_format_context->streams[_video_stream]));
 }
 
 dcp::Size
diff --git a/src/lib/player.cc b/src/lib/player.cc
index 02ae4e5c9..368d4c2ff 100644
--- a/src/lib/player.cc
+++ b/src/lib/player.cc
@@ -288,6 +288,23 @@ Player::set_approximate_size ()
 }
 
 shared_ptr<DCPVideo>
+Player::black_dcp_video (DCPTime time) const
+{
+	return shared_ptr<DCPVideo> (
+		new DCPVideo (
+			_black_image,
+			EYES_BOTH,
+			Crop (),
+			_video_container_size,
+			_video_container_size,
+			Scaler::from_id ("bicubic"),
+			Config::instance()->colour_conversions().front().conversion,
+			time
+		)
+	);
+}
+
+shared_ptr<DCPVideo>
 Player::get_video (DCPTime time, bool accurate)
 {
 	if (!_have_valid_pieces) {
@@ -296,19 +313,8 @@ Player::get_video (DCPTime time, bool accurate)
 	
 	list<shared_ptr<Piece> > ov = overlaps<VideoContent> (time);
 	if (ov.empty ()) {
-		/* No video content at this time: return a black frame */
-		return shared_ptr<DCPVideo> (
-			new DCPVideo (
-				_black_image,
-				EYES_BOTH,
-				Crop (),
-				_video_container_size,
-				_video_container_size,
-				Scaler::from_id ("bicubic"),
-				Config::instance()->colour_conversions().front().conversion,
-				time
-				)
-			);
+		/* No video content at this time */
+		return black_dcp_video (time);
 	}
 
 	/* Create a DCPVideo from the content's video at this time */
@@ -320,7 +326,9 @@ Player::get_video (DCPTime time, bool accurate)
 	assert (content);
 
 	optional<ContentVideo> dec = decoder->get_video (dcp_to_content_video (piece, time), accurate);
-	assert (dec);
+	if (!dec) {
+		return black_dcp_video (time);
+	}
 
 	dcp::Size image_size = content->scale().size (content, _video_container_size, _film->frame_size ());
 	if (_approximate_size) {
diff --git a/src/lib/player.h b/src/lib/player.h
index 852e7243f..d83045ab1 100644
--- a/src/lib/player.h
+++ b/src/lib/player.h
@@ -131,6 +131,7 @@ private:
 	VideoFrame dcp_to_content_video (boost::shared_ptr<const Piece> piece, DCPTime t) const;
 	AudioFrame dcp_to_content_audio (boost::shared_ptr<const Piece> piece, DCPTime t) const;
 	ContentTime dcp_to_content_subtitle (boost::shared_ptr<const Piece> piece, DCPTime t) const;
+	boost::shared_ptr<DCPVideo> black_dcp_video (DCPTime) const;
 
 	template<class C>
 	std::list<boost::shared_ptr<Piece> >
diff --git a/src/lib/video_decoder.cc b/src/lib/video_decoder.cc
index 6a7a62b74..bd609d168 100644
--- a/src/lib/video_decoder.cc
+++ b/src/lib/video_decoder.cc
@@ -29,7 +29,12 @@ using boost::shared_ptr;
 using boost::optional;
 
 VideoDecoder::VideoDecoder (shared_ptr<const VideoContent> c)
+#ifdef DCPOMATIC_DEBUG
+	: test_gaps (0)
+	, _video_content (c)
+#else
 	: _video_content (c)
+#endif
 {
 
 }
@@ -56,13 +61,34 @@ VideoDecoder::get_video (VideoFrame frame, bool accurate)
 
 	optional<ContentVideo> dec;
 
-	/* Now enough pass() calls will either:
+	/* Now enough pass() calls should either:
 	 *  (a) give us what we want, or
 	 *  (b) hit the end of the decoder.
 	 */
 	if (accurate) {
-		/* We are being accurate, so we want the right frame */
-		while (!decoded_video (frame) && !pass ()) {}
+		/* We are being accurate, so we want the right frame.
+		 * This could all be one statement but it's split up for clarity.
+		 */
+		while (true) {
+			if (decoded_video (frame)) {
+				/* We got what we want */
+				break;
+			}
+
+			if (pass ()) {
+				/* The decoder has nothing more for us */
+				break;
+			}
+
+			if (!_decoded_video.empty() && _decoded_video.front().frame > frame) {
+				/* We're never going to get the frame we want.  Perhaps the caller is asking
+				 * for a video frame before the content's video starts (if its audio
+				 * begins before its video, for example).
+				 */
+				break;
+			}
+		}
+
 		dec = decoded_video (frame);
 	} else {
 		/* Any frame will do: use the first one that comes out of pass() */
@@ -85,9 +111,16 @@ VideoDecoder::get_video (VideoFrame frame, bool accurate)
 void
 VideoDecoder::video (shared_ptr<const Image> image, VideoFrame frame)
 {
+	/* We should not receive the same thing twice */
+	assert (_decoded_video.empty() || frame != _decoded_video.back().frame);
+
 	/* Fill in gaps */
 	/* XXX: 3D */
+
 	while (!_decoded_video.empty () && (_decoded_video.back().frame + 1) < frame) {
+#ifdef DCPOMATIC_DEBUG
+		test_gaps++;
+#endif
 		_decoded_video.push_back (
 			ContentVideo (
 				_decoded_video.back().image,
diff --git a/src/lib/video_decoder.h b/src/lib/video_decoder.h
index 86a9489b1..8715b9714 100644
--- a/src/lib/video_decoder.h
+++ b/src/lib/video_decoder.h
@@ -41,6 +41,10 @@ public:
 		return _video_content;
 	}
 
+#ifdef DCPOMATIC_DEBUG
+	int test_gaps;
+#endif
+
 protected:
 
 	void seek (ContentTime time, bool accurate);