eventloop and abstractui debugging, lots more commenting on abstractui/eventloop implementation; minor tweaks elsewhere

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

gtk2_ardour/midi_time_axis.cc

@@ -313,7 +313,6 @@ MidiTimeAxisView::model_changed()
 
 	for (std::list<std::string>::const_iterator i = device_modes.begin();
 			i != device_modes.end(); ++i) {
-		cerr << "found custom device mode " << *i << " thread_id: " << pthread_self() << endl;
 		_custom_device_mode_selector.append_text(*i);
 	}
 

libs/ardour/ardour/midi_ui.h

@@ -5,6 +5,7 @@
 
 #include "pbd/abstract_ui.h"
 #include "pbd/signals.h"
+#include "pbd/stacktrace.h"
 
 namespace MIDI {
 	class Port;
@@ -21,8 +22,8 @@ class Session;
 
 struct MidiUIRequest : public BaseUI::BaseRequestObject {
   public:
-	MidiUIRequest () {}
+	MidiUIRequest () { }
-	~MidiUIRequest() {}
+	~MidiUIRequest() { }
 };
 
 class MidiControlUI : public AbstractUI<MidiUIRequest>

libs/ardour/midi_ui.cc

@@ -134,6 +134,8 @@ MidiControlUI::reset_ports ()
 
 	for (MIDI::Manager::PortList::const_iterator i = plist->begin(); i != plist->end(); ++i) {
 
+		cerr << "MIDI UI looking at port " << (*i)->name() << endl;
+
 		if (!(*i)->centrally_parsed()) {
 			continue;
 		}

libs/midi++2/ipmidi_port.cc

@@ -249,6 +249,7 @@ int
 IPMIDIPort::write (byte* msg, size_t msglen, timestamp_t /* ignored */) {
 
 	if (sockout) {
+		Glib::Mutex::Lock lm (write_lock);
 		if (::sendto (sockout, (char *) msg, msglen, 0, (struct sockaddr *) &addrout, sizeof(struct sockaddr_in)) < 0) {
 			::perror("sendto");
 			return -1;

libs/midi++2/jack_midi_port.cc

@@ -271,6 +271,7 @@ JackMIDIPort::write(byte * msg, size_t msglen, timestamp_t timestamp)
 				cerr << "write of " << msglen << " failed, port holds "
 					<< jack_midi_get_event_count (jack_port_get_buffer (_jack_port, _nframes_this_cycle))
 					<< endl;
+				// PBD::stacktrace (cerr, 20);
 			}
 		} else {
 			cerr << "write to JACK midi port failed: not currently in a process cycle." << endl;

libs/midi++2/midi++/ipmidi_port.h

@@ -28,6 +28,8 @@
 #include <net/if.h>
 #endif
 
+#include <glibmm/thread.h>
+
 #include <jack/types.h>
 
 #include "pbd/xml++.h"
@@ -64,7 +66,8 @@ private:
     int sockin;
     int sockout;
     struct sockaddr_in addrout;
-    
+    Glib::Mutex write_lock;    
+
     bool open_sockets (int base_port, const std::string& ifname);
     void close_sockets ();
 

libs/pbd/base_ui.cc

@@ -25,6 +25,7 @@
 #include <cstring>
 
 #include "pbd/base_ui.h"
+#include "pbd/debug.h"
 #include "pbd/pthread_utils.h"
 #include "pbd/error.h"
 #include "pbd/compose.h"
@@ -72,6 +73,7 @@ BaseUI::new_request_type ()
 void
 BaseUI::main_thread ()
 {
+	DEBUG_TRACE (DEBUG::EventLoop, string_compose ("%1: event loop running in thread %2\n", name(), pthread_self()));
 	set_event_loop_for_thread (this);
 	thread_init ();
 	_main_loop->run ();

libs/pbd/debug.cc

@@ -36,6 +36,8 @@ uint64_t PBD::DEBUG::Stateful = PBD::new_debug_bit ("stateful");
 uint64_t PBD::DEBUG::Properties = PBD::new_debug_bit ("properties");
 uint64_t PBD::DEBUG::FileManager = PBD::new_debug_bit ("filemanager");
 uint64_t PBD::DEBUG::Pool = PBD::new_debug_bit ("pool");
+uint64_t PBD::DEBUG::EventLoop = PBD::new_debug_bit ("eventloop");
+uint64_t PBD::DEBUG::AbstractUI = PBD::new_debug_bit ("abstractui");
 
 uint64_t PBD::debug_bits = 0x0;
 

libs/pbd/event_loop.cc

@@ -20,14 +20,35 @@ EventLoop::set_event_loop_for_thread (EventLoop* loop)
 	thread_event_loop.set (loop, do_not_delete_the_loop_pointer); 
 }
 
-/** Called when a sigc::trackable that was connected to using the invalidator() macro
- *  is destroyed.
- */
 void* 
 EventLoop::invalidate_request (void* data)
 {
         InvalidationRecord* ir = (InvalidationRecord*) data;
 
+	/* Some of the requests queued with an EventLoop may involve functors
+	 * that make method calls to objects whose lifetime is shorter
+	 * than the EventLoop's. We do not want to make those calls if the
+	 * object involve has been destroyed. To prevent this, we 
+	 * provide a way to invalidate those requests when the object is
+	 * destroyed.
+	 *
+	 * An object was passed to __invalidator() which added a callback to
+	 * EventLoop::invalidate_request() to its "notify when destroyed"
+	 * list. __invalidator() returned an InvalidationRecord that has been
+	 * to passed to this function as data.
+	 *
+	 * The object is currently being destroyed and so we want to
+	 * mark all requests involving this object that are queued with
+	 * any EventLoop as invalid. 
+	 *
+	 * As of April 2012, we are usign sigc::trackable as the base object
+	 * used to queue calls to ::invalidate_request() to be made upon
+	 * destruction, via its ::add_destroy_notify_callback() API. This is
+	 * not necessarily ideal, but it is very close to precisely what we
+	 * want, and many of the objects we want to do this with already
+	 * inherit (indirectly) from sigc::trackable.
+	 */
+	
         if (ir->event_loop) {
 		Glib::Mutex::Lock lm (ir->event_loop->slot_invalidation_mutex());
 		for (list<BaseRequestObject*>::iterator i = ir->requests.begin(); i != ir->requests.end(); ++i) {

libs/pbd/pbd/abstract_ui.cc

@@ -5,6 +5,7 @@
 #include "pbd/abstract_ui.h"
 #include "pbd/pthread_utils.h"
 #include "pbd/failed_constructor.h"
+#include "pbd/debug.h"
 
 #include "i18n.h"
 
@@ -17,6 +18,14 @@ template<typename RequestBuffer> void
 cleanup_request_buffer (void* ptr)
 {
         RequestBuffer* rb = (RequestBuffer*) ptr;
+	
+	/* there is the question of why we don't simply erase the request
+	 * buffer and delete it right here, since we have to take the lock
+	 * anyway.
+	 *
+	 * as of april 24th 2012, i don't have a good answer to that.
+	 */
+	 
 
         {
                 Glib::Mutex::Lock lm (rb->ui.request_buffer_map_lock);
@@ -40,10 +49,25 @@ AbstractUI<RequestObject>::AbstractUI (const string& name)
 template <typename RequestObject> void
 AbstractUI<RequestObject>::register_thread (string target_gui, pthread_t thread_id, string /*thread name*/, uint32_t num_requests)
 {
+	/* the calling thread wants to register with the thread that runs this
+	 * UI's event loop, so that it will have its own per-thread queue of
+	 * requests. this means that when it makes a request to this UI it can
+	 * do so in a realtime-safe manner (no locks).
+	 */
+
 	if (target_gui != name()) {
+		/* this UI is not the UI that the calling thread is trying to
+		   register with
+		*/
 		return;
 	}
 
+	/* the per_thread_request_buffer is a thread-private variable.
+	   See pthreads documentation for more on these, but the key
+	   thing is that it is a variable that as unique value for
+	   each thread, guaranteed.
+	*/
+
 	RequestBuffer* b = per_thread_request_buffer.get();
 
         if (b) {
@@ -52,13 +76,30 @@ AbstractUI<RequestObject>::register_thread (string target_gui, pthread_t thread_
                 return;
         }
 
+	/* create a new request queue/ringbuffer */
+
         b = new RequestBuffer (num_requests, *this);
 
 	{
+		/* add the new request queue (ringbuffer) to our map
+		   so that we can iterate over it when the time is right.
+		   This step is not RT-safe, but is assumed to be called
+		   only at thread initialization time, not repeatedly, 
+		   and so this is of little consequence.
+		*/
 		Glib::Mutex::Lock lm (request_buffer_map_lock);
 		request_buffers[thread_id] = b;
 	}
 
+	/* set this thread's per_thread_request_buffer to this new 
+	   queue/ringbuffer. remember that only this thread will
+	   get this queue when it calls per_thread_request_buffer.get()
+
+	   the second argument is a function that will be called
+	   when the thread exits, and ensures that the buffer is marked
+	   dead. it will then be deleted during a call to handle_ui_requests()
+	*/
+	
 	per_thread_request_buffer.set (b, cleanup_request_buffer<RequestBuffer>);
 }
 
@@ -68,20 +109,38 @@ AbstractUI<RequestObject>::get_request (RequestType rt)
 	RequestBuffer* rbuf = per_thread_request_buffer.get ();
 	RequestBufferVector vec;
 
+	/* see comments in ::register_thread() above for an explanation of
+	   the per_thread_request_buffer variable
+	*/
+
 	if (rbuf != 0) {
-		/* we have a per-thread FIFO, use it */
+
+		/* the calling thread has registered with this UI and therefore
+		 * we have a per-thread request queue/ringbuffer. use it. this
+		 * "allocation" of a request is RT-safe.
+		 */
 
 		rbuf->get_write_vector (&vec);
 
 		if (vec.len[0] == 0) {
+			DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: no space in per thread pool for request of type %2\n", name(), rt));
 			return 0;
 		}
 
+		DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: allocated per-thread request of type %2, caller %3\n", name(), rt, pthread_self()));
+
 		vec.buf[0]->type = rt;
                 vec.buf[0]->valid = true;
 		return vec.buf[0];
 	}
 
+	/* calling thread has not registered, so just allocate a new request on
+	 * the heap. the lack of registration implies that realtime constraints
+	 * are not at work.
+	 */
+
+	DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: allocated normal heap request of type %2, caller %3\n", name(), rt, pthread_self()));
+
 	RequestObject* req = new RequestObject;
 	req->type = rt;
 
@@ -94,7 +153,7 @@ AbstractUI<RequestObject>::handle_ui_requests ()
 	RequestBufferMapIterator i;
 	RequestBufferVector vec;
 
-	/* per-thread buffers first */
+	/* check all registered per-thread buffers first */
 
 	request_buffer_map_lock.lock ();
 
@@ -134,6 +193,8 @@ AbstractUI<RequestObject>::handle_ui_requests ()
 
 	for (i = request_buffers.begin(); i != request_buffers.end(); ) {
              if ((*i).second->dead) {
+		     DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 deleting dead per-thread request buffer for %3 @ %4\n",
+									  name(), pthread_self(), i->first, i->second));
                      delete (*i).second;
                      RequestBufferMapIterator tmp = i;
                      ++tmp;
@@ -156,11 +217,12 @@ AbstractUI<RequestObject>::handle_ui_requests ()
 
                 /* We need to use this lock, because its the one
                    returned by slot_invalidation_mutex() and protects
-                   against request invalidation.
+                   against request invalidation. 
                 */
 
                 request_buffer_map_lock.lock ();
                 if (!req->valid) {
+			DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 handling invalid heap request, type %3, deleting\n", name(), pthread_self(), req->type));
                         delete req;
                         request_buffer_map_lock.unlock ();
                         continue;
@@ -172,17 +234,48 @@ AbstractUI<RequestObject>::handle_ui_requests ()
                 */
 
                 if (req->invalidation) {
+			DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 remove request from its invalidation list\n", name(), pthread_self()));
+			
+			/* after this call, if the object referenced by the
+			 * invalidation record is deleted, it will no longer
+			 * try to mark the request as invalid.
+			 */
+
                         req->invalidation->requests.remove (req);
                 }
 
+		/* at this point, an object involved in a functor could be
+		 * deleted before we actually execute the functor. so there is
+		 * a race condition that makes the invalidation architecture
+		 * somewhat pointless. 
+		 *
+		 * really, we should only allow functors containing shared_ptr
+		 * references to objects to enter into the request queue.
+		 */
+
                 request_buffer_map_lock.unlock ();
+		
+		/* unlock the request lock while we execute the request, so
+		 * that we don't needlessly block other threads (note: not RT 
+		 * threads since they have their own queue) from making requests.
+		 */
 
 		lm.release ();
 
+		DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 execute request type %3\n", name(), pthread_self(), req->type));
+
+		/* and lets do it ... this is a virtual call so that each
+		 * specific type of UI can have its own set of requests without
+		 * some kind of central request type registration logic
+		 */
+
 		do_request (req);
 
+		DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 delete heap request type %3\n", name(), pthread_self(), req->type));
 		delete req;
 
+		/* re-acquire the list lock so that we check again */
+
 		lm.acquire();
 	}
 }
@@ -190,25 +283,53 @@ AbstractUI<RequestObject>::handle_ui_requests ()
 template <typename RequestObject> void
 AbstractUI<RequestObject>::send_request (RequestObject *req)
 {
+	/* This is called to ask a given UI to carry out a request. It may be
+	 * called from the same thread that runs the UI's event loop (see the
+	 * caller_is_self() case below), or from any other thread.
+	 */
+
 	if (base_instance() == 0) {
 		return; /* XXX is this the right thing to do ? */
 	}
 
 	if (caller_is_self ()) {
+		/* the thread that runs this UI's event loop is sending itself
+		   a request: we dispatch it immediately and inline.
+		*/
+		DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 direct dispatch of request type %3\n", name(), pthread_self(), req->type));
 		do_request (req);
 	} else {	
+
+		/* If called from a different thread, we first check to see if
+		 * the calling thread is registered with this UI. If so, there
+		 * is a per-thread ringbuffer of requests that ::get_request()
+		 * just set up a new request in. If so, all we need do here is
+		 * to advance the write ptr in that ringbuffer so that the next
+		 * request by this calling thread will use the next slot in
+		 * the ringbuffer. The ringbuffer has
+		 * single-reader/single-writer semantics because the calling
+		 * thread is the only writer, and the UI event loop is the only
+		 * reader.
+		 */
+
 		RequestBuffer* rbuf = per_thread_request_buffer.get ();
 
 		if (rbuf != 0) {
+			DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 send per-thread request type %3\n", name(), pthread_self(), req->type));
 			rbuf->increment_write_ptr (1);
 		} else {
 			/* no per-thread buffer, so just use a list with a lock so that it remains
 			   single-reader/single-writer semantics
 			*/
+			DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 send heap request type %3\n", name(), pthread_self(), req->type));
 			Glib::Mutex::Lock lm (request_list_lock);
 			request_list.push_back (req);
 		}
 
+		/* send the UI event loop thread a wakeup so that it will look
+		   at the per-thread and generic request lists.
+		*/
+
 		request_channel.wakeup ();
 	}
 }
@@ -217,6 +338,7 @@ template<typename RequestObject> void
 AbstractUI<RequestObject>::call_slot (InvalidationRecord* invalidation, const boost::function<void()>& f)
 {
 	if (caller_is_self()) {
+		DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 direct dispatch of call slot via functor @ %3, invalidation %4\n", name(), pthread_self(), &f, invalidation));
 		f ();
 		return;
 	}
@@ -227,7 +349,20 @@ AbstractUI<RequestObject>::call_slot (InvalidationRecord* invalidation, const bo
 		return;
 	}
 
+	DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 queue call-slot using functor @ %3, invalidation %4\n", name(), pthread_self(), &f, invalidation));
+
+	/* copy semantics: copy the functor into the request object */
+
 	req->the_slot = f;
+	
+	/* the invalidation record is an object which will carry out
+	 * invalidation of any requests associated with it when it is 
+	 * destroyed. it can be null. if its not null, associate this
+	 * request with the invalidation record. this allows us to
+	 * "cancel" requests submitted to the UI because they involved
+	 * a functor that uses an object that is being deleted.
+	 */
+
         req->invalidation = invalidation;
 
         if (invalidation) {

libs/pbd/pbd/base_ui.h

@@ -32,6 +32,15 @@
 #include "pbd/crossthread.h"
 #include "pbd/event_loop.h"
 
+/** A BaseUI is an abstraction designed to be used with any "user
+ * interface" (not necessarily graphical) that needs to wait on
+ * events/requests and dispatch/process them as they arrive.
+ *
+ * This implementation starts up a thread that runs a Glib main loop
+ * to wait on events/requests etc. 
+ */
+
+
 class BaseUI : virtual public sigc::trackable, public PBD::EventLoop
 {
   public:
@@ -52,7 +61,13 @@ class BaseUI : virtual public sigc::trackable, public PBD::EventLoop
 	static RequestType CallSlot;
 	static RequestType Quit;
 
+	/** start up a thread to run the main loop 
+	 */
 	void run ();
+
+	/** stop the thread running the main loop (and block
+	 *   until it exits)
+	 */
 	void quit ();
 
   protected:
@@ -62,9 +77,21 @@ class BaseUI : virtual public sigc::trackable, public PBD::EventLoop
 	Glib::RefPtr<Glib::MainLoop> _main_loop;
 	Glib::Thread*                 run_loop_thread;
 
+	/** Derived UI objects can implement thread_init()
+	 * which will be called by the event loop thread
+	 * immediately before it enters the event loop.
+	 */
+
 	virtual void thread_init () {};
+
+	/** Called when there input ready on the request_channel
+	 */
 	bool request_handler (Glib::IOCondition);
 
+	/** Derived UI objects must implement this method,
+	 * which will be called whenever there are requests
+	 * to be dealt with.
+	 */
 	virtual void handle_ui_requests () = 0;
 
   private:

libs/pbd/pbd/crossthread.h

@@ -26,20 +26,60 @@
 
 #include <glibmm/main.h>
 
+/** A simple abstraction of a mechanism of signalling one thread from another.
+ * The signaller calls ::wakeup() to tell the signalled thread to check for
+ * work to be done. 
+ *
+ * This implementation provides both ::selectable() for use in direct
+ * poll/select-based event loops, and a Glib::IOSource via ::ios() for use
+ * in Glib main loop based situations. 
+ */
+
 class CrossThreadChannel { 
   public:
-	CrossThreadChannel(bool);
+	/** if @a non_blocking is true, the channel will not cause blocking
+	 * when used in an event loop based on poll/select or the glib main
+	 * loop.
+	 */
+	CrossThreadChannel(bool non_blocking);
 	~CrossThreadChannel();
 	
+	/** Tell the listening thread that is has work to do.
+	 */
 	void wakeup();
-	int selectable() const { return fds[0]; }
+	
-
+	/* if the listening thread cares about the precise message
+	 * it is being sent, then ::deliver() can be used to send
+	 * a single byte message rather than a simple wakeup. These
+	 * two mechanisms should not be used on the same CrossThreadChannel
+	 * because there is no way to know which byte value will be used
+	 * for ::wakeup()
+	 */
         int deliver (char msg);
+
+	/** if using ::deliver() to wakeup the listening thread, then
+	 * the listener should call ::receive() to fetch the message
+	 * type from the channel.
+	 */
         int receive (char& msg);
-        
+
+	/** empty the channel of all requests.
+	 * Typically this is done as soon as input 
+	 * is noticed on the channel, because the
+	 * handler will look at a separately managed work
+	 * queue. The actual number of queued "wakeups"
+	 * in the channel will not be important.
+	 */
 	void drain ();
 	static void drain (int fd);
 
+	/** File descriptor that can be used with poll/select to
+	 * detect when wakeup() has been called on this channel.
+	 * It be marked as readable/input-ready when this condition
+	 * is true. It has already been marked non-blocking.
+	 */
+	int selectable() const { return fds[0]; }
+
 	/* glibmm 2.22 and earlier has a terrifying bug that will
 	   cause crashes whenever a Source is removed from
 	   a MainContext (including the destruction of the MainContext),
@@ -47,15 +87,17 @@ class CrossThreadChannel {
 	   the RefPtr. I (Paul) have fixed this (https://bugzilla.gnome.org/show_bug.cgi?id=561885)
 	   but in the meantime, we need a hack to get around the issue.
 	*/
-
 	Glib::RefPtr<Glib::IOSource> ios();
 	void drop_ios ();
 
+	/** returns true if the CrossThreadChannel was
+	 * correctly constructed.
+	 */
 	bool ok() const { return fds[0] >= 0 && fds[1] >= 0; }
 
   private:
 	Glib::RefPtr<Glib::IOSource>* _ios; // lazily constructed
-	int fds[2];
+	int fds[2]; // current implementation uses a pipe/fifo
 };
 
 #endif /* __pbd__crossthread_h__ */

libs/pbd/pbd/debug.h

@@ -41,6 +41,8 @@ namespace PBD {
                 extern uint64_t Properties;
 		extern uint64_t FileManager;
 		extern uint64_t Pool;
+		extern uint64_t EventLoop;
+		extern uint64_t AbstractUI;
 	}
 }
 

libs/pbd/pbd/event_loop.h

@@ -27,6 +27,15 @@
 namespace PBD
 {
 
+/** An EventLoop is as basic abstraction designed to be used with any "user
+ * interface" (not necessarily graphical) that needs to wait on
+ * events/requests and dispatch/process them as they arrive.
+ *
+ * This is a very basic class that doesn't by itself provide an actual
+ * event loop or thread. See BaseUI for the "real" object to be used
+ * when something like this is needed (it inherits from EventLoop).
+ */
+
 class EventLoop 
 {
   public:

libs/surfaces/mackie/mackie_control_protocol.cc

@@ -1081,7 +1081,7 @@ MackieControlProtocol::handle_button_event (Surface& surface, Button& button, Bu
 bool
 MackieControlProtocol::midi_input_handler (IOCondition ioc, MIDI::Port* port)
 {
-	DEBUG_TRACE (DEBUG::MidiIO, string_compose ("something happend on  %1\n", port->name()));
+	DEBUG_TRACE (DEBUG::MackieControl, string_compose ("something happend on  %1\n", port->name()));
 
 	if (ioc & ~IO_IN) {
 		return false;
@@ -1089,7 +1089,7 @@ MackieControlProtocol::midi_input_handler (IOCondition ioc, MIDI::Port* port)
 
 	if (ioc & IO_IN) {
 
-		DEBUG_TRACE (DEBUG::MidiIO, string_compose ("data available on %1\n", port->name()));
+		DEBUG_TRACE (DEBUG::MackieControl, string_compose ("data available on %1\n", port->name()));
 		framepos_t now = session->engine().frame_time();
 		port->parse (now);
 	}

libs/surfaces/mackie/surface.cc

@@ -585,7 +585,7 @@ Surface::zero_controls ()
 	if (_number == 0 && _mcp.device_info().has_two_character_display()) {
 		// any hardware-specific stuff
 		// clear 2-char display
-		show_two_char_display ("aa");
+		show_two_char_display (" ");
 	}
 
 	// and the led ring for the master strip