Various tweaks to fix playback at the end of a film (#1858).

The most questionable change here is probably how
SimpleVideoView::display_next_frame no longer re-schedules
itself if the call to get_next_frame returned AGAIN; it seems
wrong to do that when FilmViewer::idle_handler() also reschedules
itself when display_next_frame() returns AGAIN.

7 files changed, 36 insertions, 30 deletions

diff --git a/src/wx/film_viewer.cc b/src/wx/film_viewer.cc
index 7adf2ba2d..920ca3a10 100644
--- a/src/wx/film_viewer.cc
+++ b/src/wx/film_viewer.cc
@@ -137,11 +137,11 @@ FilmViewer::idle_handler ()
 		return;
 	}
 
-	if (_video_view->display_next_frame(true)) {
-		_idle_get = false;
-	} else {
+	if (_video_view->display_next_frame(true) == VideoView::AGAIN) {
 		/* get() could not complete quickly so we'll try again later */
 		signal_manager->when_idle (boost::bind(&FilmViewer::idle_handler, this));
+	} else {
+		_idle_get = false;
 	}
 }
 
@@ -346,8 +346,11 @@ FilmViewer::start ()
 	}
 
 	_playing = true;
-	_video_view->start ();
+	/* Calling start() below may directly result in Stopped being emitted, and if that
+	 * happens we want it to come after the Started signal, so do that first.
+	 */
 	Started (position());
+	_video_view->start ();
 }
 
 bool
@@ -496,7 +499,7 @@ FilmViewer::seek (DCPTime t, bool accurate)
 		/* We're going to start playing again straight away
 		   so wait for the seek to finish.
 		*/
-		while (!_video_view->display_next_frame(false)) {}
+		while (_video_view->display_next_frame(false) == VideoView::AGAIN) {}
 	}
 
 	resume ();
diff --git a/src/wx/gl_video_view.cc b/src/wx/gl_video_view.cc
index e7125501e..0d79a7562 100644
--- a/src/wx/gl_video_view.cc
+++ b/src/wx/gl_video_view.cc
@@ -405,14 +405,16 @@ catch (boost::thread_interrupted& e)
 	store_current ();
 }
 
-bool
+
+VideoView::NextFrameResult
 GLVideoView::display_next_frame (bool non_blocking)
 {
-	bool const r = get_next_frame (non_blocking);
+	NextFrameResult const r = get_next_frame (non_blocking);
 	request_one_shot ();
 	return r;
 }
 
+
 void
 GLVideoView::request_one_shot ()
 {
diff --git a/src/wx/gl_video_view.h b/src/wx/gl_video_view.h
index 1f9ed08ea..675b324fc 100644
--- a/src/wx/gl_video_view.h
+++ b/src/wx/gl_video_view.h
@@ -48,7 +48,7 @@ public:
 	void start ();
 	void stop ();
 
-	bool display_next_frame (bool);
+	NextFrameResult display_next_frame (bool);
 
 	bool vsync_enabled () const {
 		return _vsync_enabled;
diff --git a/src/wx/simple_video_view.cc b/src/wx/simple_video_view.cc
index 808f885f8..2b86ea688 100644
--- a/src/wx/simple_video_view.cc
+++ b/src/wx/simple_video_view.cc
@@ -156,19 +156,12 @@ SimpleVideoView::start ()
  *  false to ask the butler to block until it has video (unless it is suspended).
  *  @return true on success, false if we did nothing because it would have taken too long.
  */
-bool
+VideoView::NextFrameResult
 SimpleVideoView::display_next_frame (bool non_blocking)
 {
-	bool r = get_next_frame (non_blocking);
-	if (!r) {
-		if (non_blocking) {
-			/* No video available; return saying we failed */
-			return false;
-		} else {
-			/* Player was suspended; come back later */
-			signal_manager->when_idle (boost::bind(&SimpleVideoView::display_next_frame, this, false));
-			return false;
-		}
+	NextFrameResult const r = get_next_frame (non_blocking);
+	if (r != SUCCESS) {
+		return r;
 	}
 
 	update ();
@@ -179,7 +172,7 @@ SimpleVideoView::display_next_frame (bool non_blocking)
 		error_dialog (get(), e.what());
 	}
 
-	return true;
+	return SUCCESS;
 }
 
 void
diff --git a/src/wx/simple_video_view.h b/src/wx/simple_video_view.h
index 323047ada..93b9c6e3d 100644
--- a/src/wx/simple_video_view.h
+++ b/src/wx/simple_video_view.h
@@ -39,7 +39,7 @@ public:
 
 	void update ();
 	void start ();
-	bool display_next_frame (bool non_blocking);
+	NextFrameResult display_next_frame (bool non_blocking);
 
 private:
 	void set_image (boost::shared_ptr<const Image> image) {
diff --git a/src/wx/video_view.cc b/src/wx/video_view.cc
index 36dbef93a..7805a1fb3 100644
--- a/src/wx/video_view.cc
+++ b/src/wx/video_view.cc
@@ -52,18 +52,19 @@ VideoView::clear ()
 
 /** Could be called from any thread.
  *  @param non_blocking true to return false quickly if no video is available quickly.
- *  @return false if we gave up because it would take too long, otherwise true.
+ *  @return FAIL if there's no frame, AGAIN if the method should be called again, or SUCCESS
+ *  if there is a frame.
  */
-bool
+VideoView::NextFrameResult
 VideoView::get_next_frame (bool non_blocking)
 {
 	if (length() == dcpomatic::DCPTime()) {
-		return true;
+		return FAIL;
 	}
 
 	shared_ptr<Butler> butler = _viewer->butler ();
 	if (!butler) {
-		return false;
+		return FAIL;
 	}
 	add_get ();
 
@@ -75,8 +76,8 @@ VideoView::get_next_frame (bool non_blocking)
 		if (e.code == Butler::Error::DIED) {
 			LOG_ERROR ("Butler died with %1", e.summary());
 		}
-		if (!pv.first && e.code == Butler::Error::AGAIN) {
-			return false;
+		if (!pv.first) {
+			return e.code == Butler::Error::AGAIN ? AGAIN : FAIL;
 		}
 		_player_video = pv;
 	} while (
@@ -90,7 +91,7 @@ VideoView::get_next_frame (bool non_blocking)
 		++_errored;
 	}
 
-	return true;
+	return SUCCESS;
 }
 
 dcpomatic::DCPTime
diff --git a/src/wx/video_view.h b/src/wx/video_view.h
index 2b4bd4717..e5fbb671d 100644
--- a/src/wx/video_view.h
+++ b/src/wx/video_view.h
@@ -50,8 +50,15 @@ public:
 	virtual void start ();
 	/** Called when playback stops */
 	virtual void stop () {}
+
+	enum NextFrameResult {
+		FAIL,
+		AGAIN,
+		SUCCESS
+	};
+
 	/** Get the next frame and display it; used after seek */
-	virtual bool display_next_frame (bool) = 0;
+	virtual NextFrameResult display_next_frame (bool) = 0;
 
 	void clear ();
 	bool reset_metadata (boost::shared_ptr<const Film> film, dcp::Size player_video_container_size);
@@ -112,7 +119,7 @@ public:
 	}
 
 protected:
-	bool get_next_frame (bool non_blocking);
+	NextFrameResult get_next_frame (bool non_blocking);
 	boost::optional<int> time_until_next_frame () const;
 	dcpomatic::DCPTime one_video_frame () const;