Various cleanups and thread-safety.

6 files changed, 69 insertions, 26 deletions

diff --git a/src/wx/film_viewer.cc b/src/wx/film_viewer.cc
index dcbfedc81..df66a8ade 100644
--- a/src/wx/film_viewer.cc
+++ b/src/wx/film_viewer.cc
@@ -334,6 +334,8 @@ FilmViewer::stop ()
 	_playing = false;
 	_video_view->stop ();
 	Stopped (position());
+
+	_video_view->rethrow ();
 	return true;
 }
 
@@ -379,6 +381,8 @@ FilmViewer::film_change (ChangeType type, Film::Property p)
 		recreate_butler ();
 	} else if (p == Film::VIDEO_FRAME_RATE) {
 		_video_view->set_video_frame_rate (_film->video_frame_rate());
+	} else if (p == Film::THREE_D) {
+		_video_view->set_three_d (_film->three_d());
 	}
 }
 
@@ -636,11 +640,21 @@ FilmViewer::set_pad_black (bool p)
 	_pad_black = p;
 }
 
-/* May be called from a non-UI thread */
+/** Called when a player has finished the current film.
+ *  May be called from a non-UI thread.
+ */
 void
-FilmViewer::emit_finished ()
+FilmViewer::finished ()
 {
-	emit (boost::bind(boost::ref(Finished)));
+	emit (boost::bind(&FilmViewer::ui_finished, this));
+}
+
+/** Called by finished() in the UI thread */
+void
+FilmViewer::ui_finished ()
+{
+	stop ();
+	Finished ();
 }
 
 int
diff --git a/src/wx/film_viewer.h b/src/wx/film_viewer.h
index ec481f34e..60cde60d0 100644
--- a/src/wx/film_viewer.h
+++ b/src/wx/film_viewer.h
@@ -131,13 +131,13 @@ public:
 	ClosedCaptionsDialog* closed_captions_dialog () const {
 		return _closed_captions_dialog;
 	}
+	void finished ();
 
 	boost::signals2::signal<void (boost::weak_ptr<PlayerVideo>)> ImageChanged;
 	boost::signals2::signal<void (dcpomatic::DCPTime)> Started;
 	boost::signals2::signal<void (dcpomatic::DCPTime)> Stopped;
 	/** While playing back we reached the end of the film (emitted from GUI thread) */
 	boost::signals2::signal<void ()> Finished;
-	void emit_finished ();
 
 	boost::signals2::signal<bool ()> PlaybackPermitted;
 
@@ -152,6 +152,7 @@ private:
 	void recreate_butler ();
 	void config_changed (Config::Property);
 	void film_length_change ();
+	void ui_finished ();
 
 	dcpomatic::DCPTime uncorrected_time () const;
 	Frame average_latency () const;
diff --git a/src/wx/gl_video_view.cc b/src/wx/gl_video_view.cc
index ce152787a..869d555cb 100644
--- a/src/wx/gl_video_view.cc
+++ b/src/wx/gl_video_view.cc
@@ -133,8 +133,11 @@ check_gl_error (char const * last)
 void
 GLVideoView::update ()
 {
-	if (!_canvas->IsShownOnScreen()) {
-		return;
+	{
+		boost::mutex::scoped_lock lm (_canvas_mutex);
+		if (!_canvas->IsShownOnScreen()) {
+			return;
+		}
 	}
 	request_one_shot ();
 }
@@ -155,16 +158,22 @@ GLVideoView::draw (Position<int> inter_position, dcp::Size inter_size)
 	check_gl_error ("glDisable GL_DEPTH_TEST");
 	glBlendFunc (GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA);
 
-	if (_canvas->GetSize().x < 64 || _canvas->GetSize().y < 0) {
+	wxSize canvas_size;
+	{
+		boost::mutex::scoped_lock lm (_canvas_mutex);
+		canvas_size = _canvas->GetSize ();
+	}
+
+	if (canvas_size.GetWidth() < 64 || canvas_size.GetHeight() < 0) {
 		return;
 	}
 
-	glViewport (0, 0, _canvas->GetSize().x, _canvas->GetSize().y);
+	glViewport (0, 0, canvas_size.GetWidth(), canvas_size.GetHeight());
 	check_gl_error ("glViewport");
 	glMatrixMode (GL_PROJECTION);
 	glLoadIdentity ();
 
-	gluOrtho2D (0, _canvas->GetSize().x, _canvas->GetSize().y, 0);
+	gluOrtho2D (0, canvas_size.GetWidth(), canvas_size.GetHeight(), 0);
 	check_gl_error ("gluOrtho2d");
 	glMatrixMode (GL_MODELVIEW);
 	glLoadIdentity ();
@@ -197,8 +206,6 @@ GLVideoView::draw (Position<int> inter_position, dcp::Size inter_size)
 		glEnd ();
 	}
 
-	wxSize const canvas_size = _canvas->GetSize ();
-
 	if (!_viewer->pad_black() && out_size.width < canvas_size.GetWidth()) {
 		glBegin (GL_QUADS);
 		/* XXX: these colours are right for GNOME; may need adjusting for other OS */
@@ -241,6 +248,8 @@ GLVideoView::draw (Position<int> inter_position, dcp::Size inter_size)
 	}
 
 	glFlush();
+
+	boost::mutex::scoped_lock lm (_canvas_mutex);
 	_canvas->SwapBuffers();
 }
 
@@ -291,9 +300,11 @@ void
 GLVideoView::thread ()
 try
 {
-	/* XXX_b: check all calls and signal emissions in this method & protect them if necessary */
-	_context = new wxGLContext (_canvas);
-	_canvas->SetCurrent (*_context);
+	{
+		boost::mutex::scoped_lock lm (_canvas_mutex);
+		_context = new wxGLContext (_canvas); //local
+		_canvas->SetCurrent (*_context);
+	}
 
 	while (true) {
 		boost::mutex::scoped_lock lm (_playing_mutex);
@@ -309,12 +320,12 @@ try
 			dcpomatic::DCPTime const next = position() + one_video_frame();
 
 			if (next >= length()) {
-				_viewer->stop ();
-				_viewer->emit_finished ();
+				_viewer->finished ();
 				continue;
 			}
 
 			get_next_frame (false);
+			//--
 			set_image (player_video().first->image(bind(&PlayerVideo::force, _1, AV_PIX_FMT_RGB24), false, true));
 			inter_position = player_video().first->inter_position();
 			inter_size = player_video().first->inter_size();
@@ -330,13 +341,13 @@ try
 		dcpomatic_sleep_milliseconds (time_until_next_frame());
 	}
 
-	delete _context;
+	/* XXX: leaks _context, but that seems preferable to deleting it here
+	 * without also deleting the wxGLCanvas.
+	 */
 }
 catch (boost::thread_interrupted& e)
 {
-	/* XXX_b: store exceptions here */
-	delete _context;
-	return;
+	store_current ();
 }
 
 bool
diff --git a/src/wx/gl_video_view.h b/src/wx/gl_video_view.h
index 4f509049b..827b12861 100644
--- a/src/wx/gl_video_view.h
+++ b/src/wx/gl_video_view.h
@@ -57,6 +57,10 @@ private:
 	void create ();
 	void check_for_butler_errors ();
 
+	/* Mutex for use of _canvas; it's only contended when our ::thread
+	   is started up so this may be overkill.
+	 */
+	boost::mutex _canvas_mutex;
 	wxGLCanvas* _canvas;
 	wxGLContext* _context;
 
diff --git a/src/wx/video_view.cc b/src/wx/video_view.cc
index 5dd857fab..ee14f4a33 100644
--- a/src/wx/video_view.cc
+++ b/src/wx/video_view.cc
@@ -23,6 +23,8 @@
 #include "film_viewer.h"
 #include "lib/butler.h"
 
+using boost::shared_ptr;
+
 VideoView::VideoView (FilmViewer* viewer)
 	: _viewer (viewer)
 #ifdef DCPOMATIC_VARIANT_SWAROOP
@@ -31,6 +33,7 @@ VideoView::VideoView (FilmViewer* viewer)
 	, _state_timer ("viewer")
 	, _video_frame_rate (0)
 	, _eyes (EYES_LEFT)
+	, _three_d (false)
 	, _dropped (0)
 	, _gets (0)
 {
@@ -45,30 +48,32 @@ VideoView::clear ()
 	_player_video.second = dcpomatic::DCPTime ();
 }
 
-/** @param non_blocking true to return false quickly if no video is available quickly.
+/** 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.
  */
 bool
 VideoView::get_next_frame (bool non_blocking)
 {
-	if (_length == dcpomatic::DCPTime()) {
+	if (length() == dcpomatic::DCPTime()) {
 		return true;
 	}
 
-	DCPOMATIC_ASSERT (_viewer->butler());
+	shared_ptr<Butler> butler = _viewer->butler ();
+	DCPOMATIC_ASSERT (butler);
 	add_get ();
 
 	boost::mutex::scoped_lock lm (_mutex);
 
 	do {
 		Butler::Error e;
-		_player_video = _viewer->butler()->get_video (!non_blocking, &e);
+		_player_video = butler->get_video (!non_blocking, &e);
 		if (!_player_video.first && e == Butler::AGAIN) {
 			return false;
 		}
 	} while (
 		_player_video.first &&
-		_viewer->film()->three_d() &&
+		_three_d &&
 		_eyes != _player_video.first->eyes() &&
 		_player_video.first->eyes() != EYES_BOTH
 		);
diff --git a/src/wx/video_view.h b/src/wx/video_view.h
index fd2684e41..5d5d33163 100644
--- a/src/wx/video_view.h
+++ b/src/wx/video_view.h
@@ -24,16 +24,18 @@
 #include "lib/dcpomatic_time.h"
 #include "lib/timer.h"
 #include "lib/types.h"
+#include "lib/exception_store.h"
 #include <boost/shared_ptr.hpp>
 #include <boost/signals2.hpp>
 #include <boost/thread.hpp>
+#include <boost/noncopyable.hpp>
 
 class Image;
 class wxWindow;
 class FilmViewer;
 class PlayerVideo;
 
-class VideoView
+class VideoView : public ExceptionStore, public boost::noncopyable
 {
 public:
 	VideoView (FilmViewer* viewer);
@@ -93,6 +95,11 @@ public:
 		_eyes = eyes;
 	}
 
+	void set_three_d (bool t) {
+		boost::mutex::scoped_lock lm (_mutex);
+		_three_d = t;
+	}
+
 protected:
 	/* XXX_b: to remove */
 	friend class FilmViewer;
@@ -145,6 +152,7 @@ private:
 	/** length of the film we are playing, or 0 if there is none */
 	dcpomatic::DCPTime _length;
 	Eyes _eyes;
+	bool _three_d;
 
 	int _dropped;
 	int _gets;