more tweaks to MTC slave code (still not functional), including removing race conditions when resetting slave state; make Session catch on its own saved preferences, which has not been happening; make switching sync sources avoid race conditions

git-svn-id: svn://localhost/ardour2/branches/3.0@6269 d708f5d6-7413-0410-9779-e7cbd77b26cf

libs/ardour/ardour/session.h

@@ -175,7 +175,7 @@ class Session : public PBD::StatefulDestructible, public boost::noncopyable
 		void*        ptr;
 		bool         yes_or_no;
 		nframes64_t  target2_frame;
-		SyncSource   sync_source;
+		Slave*       slave;
 		Route*       route;
 	    };
 
@@ -584,7 +584,7 @@ class Session : public PBD::StatefulDestructible, public boost::noncopyable
 	static sigc::signal<void> TimecodeOffsetChanged;
 
         std::vector<SyncSource> get_available_sync_options() const;
-	void   request_sync_source (SyncSource);
+	void   request_sync_source (Slave*);
         bool   synced_to_jack() const { return config.get_external_sync() && config.get_sync_source() == JACK; }
 
 	double transport_speed() const { return _transport_speed; }
@@ -1104,8 +1104,9 @@ class Session : public PBD::StatefulDestructible, public boost::noncopyable
 	void track_slave_state(float slave_speed, nframes_t slave_transport_frame, nframes_t this_delta);
 	void follow_slave_silently(nframes_t nframes, float slave_speed);
 
-        void use_sync_source (SyncSource);
+        void switch_to_sync_source (SyncSource); /* !RT context */
-        void drop_sync_source ();
+        void drop_sync_source ();  /* !RT context */
+        void use_sync_source (Slave*); /* RT context */
 
         bool post_export_sync;
 	nframes_t post_export_position;

libs/ardour/ardour/slave.h

@@ -22,6 +22,8 @@
 
 #include <vector>
 
+#include <glibmm/thread.h>
+
 #include <jack/jack.h>
 
 #include <sigc++/signal.h>
@@ -241,12 +243,14 @@ class MTC_Slave : public Slave, public sigc::trackable {
 	bool        can_notify_on_unknown_rate;
 	PIController* pic;
 
-	SafeTime    current;
+	static const int frame_tolerance;
-	nframes_t   mtc_frame;               /* current time */
+
-	nframes_t   last_inbound_frame;      /* when we got it; audio clocked */
+	SafeTime       current;
-	MIDI::byte  last_mtc_fps_byte;
+	nframes_t      mtc_frame;               /* current time */
-	nframes64_t window_begin;
+	nframes_t      last_inbound_frame;      /* when we got it; audio clocked */
-	nframes64_t window_end;
+	MIDI::byte     last_mtc_fps_byte;
+	nframes64_t    window_begin;
+	nframes64_t    window_end;
 	nframes64_t    last_mtc_timestamp;
 	nframes64_t    last_mtc_frame;
 	bool           did_reset_tc_format;
@@ -256,8 +260,13 @@ class MTC_Slave : public Slave, public sigc::trackable {
 	size_t         speed_accumulator_cnt;
 	bool           have_first_speed_accumulator;
 	double         average_speed;
+	Glib::Mutex    reset_lock;
+	bool           reset_pending;
 
 	void reset ();
+	void queue_reset ();
+	void maybe_reset ();
+
 	void update_mtc_qtr (MIDI::Parser&, int, nframes_t);
 	void update_mtc_time (const MIDI::byte *, bool, nframes_t);
 	void update_mtc_status (MIDI::MTC_Status);

libs/ardour/jack_slave.cc

@@ -18,8 +18,8 @@
 */
 
 #include <iostream>
+#include <cerrno>
 
-#include <errno.h>
 #include <jack/jack.h>
 #include <jack/transport.h>
 

libs/ardour/mtc_slave.cc

@@ -41,6 +41,16 @@ using namespace sigc;
 using namespace MIDI;
 using namespace PBD;
 
+/* length (in timecode frames) of the "window" that we consider legal given receipt of
+   a given timecode position. Ardour will try to chase within this window, and will
+   stop+locate+wait+chase if timecode arrives outside of it. The window extends entirely
+   in the current direction of motion, so if any timecode arrives that is before the most
+   recently received position (and without the direction of timecode reversing too), we
+   will stop+locate+wait+chase.
+*/
+
+const int MTC_Slave::frame_tolerance = 2;
+
 MTC_Slave::MTC_Slave (Session& s, MIDI::Port& p)
 	: session (s)
 {
@@ -71,8 +81,8 @@ MTC_Slave::~MTC_Slave()
 bool 
 MTC_Slave::give_slave_full_control_over_transport_speed() const
 {
-	return true; // for PiC control */
+	// return true; // for PiC control */
-	// return false; // for Session-level computed varispeed
+	return false; // for Session-level computed varispeed
 }
 
 void
@@ -84,8 +94,6 @@ MTC_Slave::rebind (MIDI::Port& p)
 
 	port = &p;
 
-	cerr << "Bind to port MTC messages\n";
-
 	connections.push_back (port->input()->mtc_time.connect (mem_fun (*this, &MTC_Slave::update_mtc_time)));
 	connections.push_back (port->input()->mtc_qtr.connect (mem_fun (*this, &MTC_Slave::update_mtc_qtr)));
 	connections.push_back (port->input()->mtc_status.connect (mem_fun (*this, &MTC_Slave::update_mtc_status)));
@@ -94,6 +102,8 @@ MTC_Slave::rebind (MIDI::Port& p)
 void
 MTC_Slave::update_mtc_qtr (Parser& /*p*/, int which_qtr, nframes_t now)
 {
+	maybe_reset ();
+
 	DEBUG_TRACE (DEBUG::MTC, string_compose ("qtr frame %1 at %2\n", which_qtr, now));
 	last_inbound_frame = now;
 }
@@ -102,9 +112,14 @@ void
 MTC_Slave::update_mtc_time (const byte *msg, bool was_full, nframes_t now)
 {
 	/* "now" can be zero if this is called from a context where we do not have or do not want
-	   to use a timestamp indicating when this MTC time was received.
+	   to use a timestamp indicating when this MTC time was received. example: when we received
+	   a locate command via MMC.
 	*/
 
+	if (now) {
+		maybe_reset ();
+	}
+
 	Timecode::Time timecode;
 	TimecodeFormat tc_format;
 	bool reset_tc = true;
@@ -174,8 +189,7 @@ MTC_Slave::update_mtc_time (const byte *msg, bool was_full, nframes_t now)
 		session.request_locate (mtc_frame, false);
 		session.request_transport_speed (0);
 		update_mtc_status (MIDI::MTC_Stopped);
-		window_root = mtc_frame;
+		reset_window (mtc_frame);
-		
 		reset ();
 
 	} else {
@@ -265,6 +279,12 @@ MTC_Slave::process_apparent_speed (double this_speed)
 {
 	DEBUG_TRACE (DEBUG::MTC, string_compose ("speed cnt %1 sz %2 have %3\n", speed_accumulator_cnt, speed_accumulator_size, have_first_speed_accumulator));
 
+	/* clamp to an expected range */
+
+	if (this_speed > 4.0 || this_speed < -4.0) {
+		this_speed = average_speed;
+	}
+
 	if (speed_accumulator_cnt >= speed_accumulator_size) {
 		have_first_speed_accumulator = true;
 		speed_accumulator_cnt = 0;
@@ -385,9 +405,8 @@ MTC_Slave::speed_and_position (double& speed, nframes64_t& pos)
 		pos = last.position;
 		session.request_locate (pos, false);
 		session.request_transport_speed (0);
-		update_mtc_status (MIDI::MTC_Stopped);
+		queue_reset ();
-		reset();
+		DEBUG_TRACE (DEBUG::MTC, "MTC not seen for 1/4 second - reset pending\n");
-		DEBUG_TRACE (DEBUG::MTC, "MTC not seen for 1/4 second - reset\n");
 		return false;
 	}
 
@@ -426,14 +445,29 @@ MTC_Slave::resolution() const
 	return (nframes_t) session.frames_per_timecode_frame();
 }
 
+void
+MTC_Slave::queue_reset ()
+{
+	Glib::Mutex::Lock lm (reset_lock);
+	reset_pending++;
+}
+
+void
+MTC_Slave::maybe_reset ()
+{
+	reset_lock.lock ();
+
+	if (reset_pending) {
+		reset ();
+		reset_pending = 0;
+	} 
+
+	reset_lock.unlock ();
+}
+
 void
 MTC_Slave::reset ()
 {
-	/* XXX massive thread safety issue here. MTC could
-	   be being updated as we call this. but this
-	   supposed to be a realtime-safe call.
-	*/
-
 	port->input()->reset_mtc_state ();
 
 	last_inbound_frame = 0;
@@ -458,15 +492,48 @@ MTC_Slave::reset ()
 void
 MTC_Slave::reset_window (nframes64_t root)
 {
-	window_begin = root;
+	
+	/* if we're waiting for the master to catch us after seeking ahead, keep the window
+	   of acceptable MTC frames wide open. otherwise, shrink it down to just 2 video frames
+	   ahead of the window root (taking direction into account).
+	*/
 
-	if (session.slave_state() == Session::Running) {
+	switch (port->input()->mtc_running()) {
-		window_end = root + (session.frames_per_timecode_frame() * 2);
+	case MTC_Forward:
-	} else {
+		window_begin = root;
-		window_end = root + seekahead_distance ();
+		if (session.slave_state() == Session::Running) {
+			window_end = root + (session.frames_per_timecode_frame() * frame_tolerance);
+		} else {
+			window_end = root + seekahead_distance ();
+		}
+		DEBUG_TRACE (DEBUG::MTC, string_compose ("legal MTC window now %1 .. %2\n", window_begin, window_end));
+		break;
+
+	case MTC_Backward:
+		if (session.slave_state() == Session::Running) {
+			nframes_t d = session.frames_per_timecode_frame() * frame_tolerance;
+			if (root > d) {
+				window_begin = root - d;
+				window_end = root;
+			} else {
+				window_begin = 0;
+			}
+		} else {
+			nframes_t d = seekahead_distance ();
+			if (root > d) {
+				window_begin = root - d;
+			} else {
+				window_begin = 0;
+			}
+		}
+		window_end = root;
+		DEBUG_TRACE (DEBUG::MTC, string_compose ("legal MTC window now %1 .. %2\n", window_begin, window_end));
+		break;
+		
+	default:
+		/* do nothing */
+		break;
 	}
-
-	DEBUG_TRACE (DEBUG::MTC, string_compose ("legal MTC window now %1 .. %2\n", window_begin, window_end));
 }
 
 nframes64_t

libs/ardour/session.cc

@@ -538,6 +538,7 @@ Session::when_engine_running ()
 	BootMessage (_("Using configuration"));
 
 	Config->map_parameters (bind (mem_fun (*this, &Session::config_changed), false));
+	config.map_parameters (bind (mem_fun (*this, &Session::config_changed), true));
 
 	/* every time we reconnect, recompute worst case output latencies */
 

libs/ardour/session_events.cc

@@ -395,7 +395,7 @@ Session::process_event (Event* ev)
 		break;
 
 	case Event::SetSyncSource:
-		use_sync_source (ev->sync_source);
+		use_sync_source (ev->slave);
 		break;
 
 	case Event::Audition:

libs/ardour/session_process.cc

@@ -572,7 +572,7 @@ Session::follow_slave (nframes_t nframes)
 			if (_slave->give_slave_full_control_over_transport_speed()) {
 				set_transport_speed (slave_speed, false, false);
 			} else {
-				float adjusted_speed = slave_speed + (delta /  float(_current_frame_rate));
+				float adjusted_speed = slave_speed + (1.5 * (delta /  float(_current_frame_rate)));
 				request_transport_speed (adjusted_speed);
 				DEBUG_TRACE (DEBUG::Slave, string_compose ("adjust using %1 towards %2 ratio %3 current %4 slave @ %5\n",
 									   delta, adjusted_speed, adjusted_speed/slave_speed, _transport_speed,

libs/ardour/session_state.cc

@@ -58,6 +58,7 @@
 #include "midi++/port.h"
 
 #include "pbd/boost_debug.h"
+#include "pbd/enumwriter.h"
 #include "pbd/error.h"
 #include "pbd/pathscanner.h"
 #include "pbd/pthread_utils.h"
@@ -3212,7 +3213,7 @@ Session::config_changed (std::string p, bool ours)
 		if (!config.get_external_sync()) {
 			drop_sync_source ();
 		} else {
-			use_sync_source (config.get_sync_source());
+			switch_to_sync_source (config.get_sync_source());
 		}
 	} else if (p == "remote-model") {
 		set_remote_control_ids ();

libs/ardour/session_transport.cc

@@ -79,14 +79,14 @@ Session::request_input_change_handling ()
 }
 
 void
-Session::request_sync_source (SyncSource src)
+Session::request_sync_source (Slave* new_slave)
 {
 	Event* ev = new Event (Event::SetSyncSource, Event::Add, Event::Immediate, 0, 0.0);
 	bool seamless;
 
 	seamless = Config->get_seamless_loop ();
 
-	if (src == JACK) {
+	if (dynamic_cast<JACK_Slave*>(new_slave)) {
 		/* JACK cannot support seamless looping at present */
 		Config->set_seamless_loop (false);
 	} else {
@@ -97,7 +97,7 @@ Session::request_sync_source (SyncSource src)
 	/* save value of seamless from before the switch */
 	_was_seamless = seamless;
 
-	ev->sync_source = src;
+	ev->slave = new_slave;
 	queue_event (ev);
 }
 
@@ -1156,17 +1156,16 @@ Session::reset_rf_scale (nframes_t motion)
 }
 
 void
-Session::drop_sync_source ()
+Session::use_sync_source (Slave* new_slave)
 {
+	/* Runs in process() context */
+
 	bool non_rt_required = false;
 
-	if (_transport_speed) {
+	/* XXX this deletion is problematic because we're in RT context */
-		error << _("please stop the transport before adjusting slave settings") << endmsg;
-		return;
-	}
 
 	delete _slave;
-	_slave = 0;
+	_slave = new_slave;
 
 	boost::shared_ptr<DiskstreamList> dsl = diskstreams.reader();
 	for (DiskstreamList::iterator i = dsl->begin(); i != dsl->end(); ++i) {
@@ -1174,7 +1173,7 @@ Session::drop_sync_source ()
 			if ((*i)->realtime_set_speed ((*i)->speed(), true)) {
 				non_rt_required = true;
 			}
-			(*i)->set_slaved (0);
+			(*i)->set_slaved (_slave != 0);
 		}
 	}
 
@@ -1187,24 +1186,27 @@ Session::drop_sync_source ()
 }
 
 void
-Session::use_sync_source (SyncSource src)
+Session::drop_sync_source ()
 {
-	bool reverse = false;
+	request_sync_source (0);
-	bool non_rt_required = false;
+}
 
-	if (_transport_speed) {
+void
-		error << _("please stop the transport before adjusting slave settings") << endmsg;
+Session::switch_to_sync_source (SyncSource src)
-		return;
+{
-	}
+	Slave* new_slave;
 
-	delete _slave;
+	DEBUG_TRACE (DEBUG::Slave, string_compose ("Setting up sync source %1\n", enum_2_string (src)));
-	_slave = 0;
 
 	switch (src) {
 	case MTC:
+		if (_slave && dynamic_cast<MTC_Slave*>(_slave)) {
+			return;
+		}
+
 		if (_mtc_port) {
 			try {
-				_slave = new MTC_Slave (*this, *_mtc_port);
+				new_slave = new MTC_Slave (*this, *_mtc_port);
 			}
 
 			catch (failed_constructor& err) {
@@ -1218,9 +1220,13 @@ Session::use_sync_source (SyncSource src)
 		break;
 
 	case MIDIClock:
+		if (_slave && dynamic_cast<MIDIClock_Slave*>(_slave)) {
+			return;
+		}
+
 		if (_midi_clock_port) {
 			try {
-				_slave = new MIDIClock_Slave (*this, *_midi_clock_port, 24);
+				new_slave = new MIDIClock_Slave (*this, *_midi_clock_port, 24);
 			}
 
 			catch (failed_constructor& err) {
@@ -1234,31 +1240,19 @@ Session::use_sync_source (SyncSource src)
 		break;
 
 	case JACK:
-		_slave = new JACK_Slave (_engine.jack());
+		if (_slave && dynamic_cast<JACK_Slave*>(_slave)) {
-		break;
+			return;
+		}
 
+		new_slave = new JACK_Slave (_engine.jack());
+		break;
+		
+	default:
+		new_slave = 0;
+		break;
 	};
 
-	boost::shared_ptr<DiskstreamList> dsl = diskstreams.reader();
+	request_sync_source (new_slave);
-	for (DiskstreamList::iterator i = dsl->begin(); i != dsl->end(); ++i) {
-		if (!(*i)->hidden()) {
-			if ((*i)->realtime_set_speed ((*i)->speed(), true)) {
-				non_rt_required = true;
-			}
-			(*i)->set_slaved (_slave);
-		}
-	}
-
-	if (reverse) {
-		reverse_diskstream_buffers ();
-	}
-
-	if (non_rt_required) {
-		add_post_transport_work (PostTransportSpeed);
-		_butler->schedule_transport_work ();
-	}
-
-	set_dirty();
 }
 
 void

libs/midi++2/mtc.cc

@@ -70,6 +70,8 @@ Parser::possible_mtc (byte *sysex_buf, size_t msglen)
 void
 Parser::reset_mtc_state ()
 {
+	/* MUST REMAIN RT-SAFE */
+
 	_mtc_forward = false;
 	_mtc_running = MTC_Stopped;
 	_mtc_locked = false;