Support multiple readers for MIDI source/model

Fixes the multiple reader issue #6541 properly without resorting to a
linear search kludge.

All the read state has been pulled out into a MidiCursor which the
caller is required to pass.  The playlist keeps cursors for all the
regions it is reading, any number of cursors are allowed at a time.

MidiCursor should probably be made a smarter and more fool-proof
object (and/or possibly merged with some of the other tracker/fixer
stuff) but for now I wanted to keep it simple.

11 files changed, 123 insertions, 92 deletions

diff --git a/libs/ardour/ardour/midi_cursor.h b/libs/ardour/ardour/midi_cursor.h
new file mode 100644
index 0000000000..5cb89c87f5
--- /dev/null
+++ b/libs/ardour/ardour/midi_cursor.h
@@ -0,0 +1,56 @@
+/*
+    Copyright (C) 2016 Paul Davis
+    Author: David Robillard
+
+    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
+    the Free Software Foundation; either version 2 of the License, or
+    (at your option) any later version.
+
+    This program is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with this program; if not, write to the Free Software
+    Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+*/
+
+#ifndef __ardour_midi_cursor_h__
+#define __ardour_midi_cursor_h__
+
+#include <set>
+
+#include <boost/utility.hpp>
+
+#include "ardour/types.h"
+#include "evoral/Beats.hpp"
+#include "evoral/Sequence.hpp"
+#include "pbd/signals.h"
+
+namespace ARDOUR {
+
+struct MidiCursor : public boost::noncopyable {
+	MidiCursor() : last_read_end(0) {}
+
+	void connect(PBD::Signal1<void, bool>& invalidated) {
+		connections.drop_connections();
+		invalidated.connect_same_thread(
+			connections, boost::bind(&MidiCursor::invalidate, this, _1));
+	}
+
+	void invalidate(bool preserve_notes) {
+		iter.invalidate(preserve_notes ? &active_notes : NULL);
+		last_read_end = 0;
+	}
+
+	Evoral::Sequence<Evoral::Beats>::const_iterator        iter;
+	std::set<Evoral::Sequence<Evoral::Beats>::WeakNotePtr> active_notes;
+	framepos_t                                             last_read_end;
+	PBD::ScopedConnectionList                              connections;
+};
+
+}
+
+#endif /* __ardour_midi_cursor_h__ */
diff --git a/libs/ardour/ardour/midi_model.h b/libs/ardour/ardour/midi_model.h
index c5467cc58d..cdf7b3fd3b 100644
--- a/libs/ardour/ardour/midi_model.h
+++ b/libs/ardour/ardour/midi_model.h
@@ -292,8 +292,6 @@ public:
 	void insert_silence_at_start (TimeType);
 	void transpose (NoteDiffCommand *, const NotePtr, int);
 
-	std::set<WeakNotePtr>& active_notes() { return _active_notes; }
-
 protected:
 	int resolve_overlaps_unlocked (const NotePtr, void* arg = 0);
 
@@ -327,8 +325,6 @@ private:
 	// We cannot use a boost::shared_ptr here to avoid a retain cycle
 	boost::weak_ptr<MidiSource> _midi_source;
 	InsertMergePolicy _insert_merge_policy;
-
-	std::set<WeakNotePtr> _active_notes;
 };
 
 } /* namespace ARDOUR */
diff --git a/libs/ardour/ardour/midi_playlist.h b/libs/ardour/ardour/midi_playlist.h
index ad0a812cba..3c031a994f 100644
--- a/libs/ardour/ardour/midi_playlist.h
+++ b/libs/ardour/ardour/midi_playlist.h
@@ -26,6 +26,7 @@
 #include <boost/utility.hpp>
 
 #include "ardour/ardour.h"
+#include "ardour/midi_cursor.h"
 #include "ardour/midi_model.h"
 #include "ardour/midi_state_tracker.h"
 #include "ardour/note_fixer.h"
@@ -112,12 +113,14 @@ public:
 
 protected:
 	void remove_dependents (boost::shared_ptr<Region> region);
+	void region_going_away (boost::weak_ptr<Region> region);
 
 private:
 	typedef Evoral::Note<Evoral::Beats> Note;
 	typedef Evoral::Event<framepos_t>   Event;
 
 	struct RegionTracker : public boost::noncopyable {
+		MidiCursor       cursor;   ///< Cursor (iterator and read state)
 		MidiStateTracker tracker;  ///< Active note tracker
 		NoteFixer        fixer;    ///< Edit compensation
 	};
diff --git a/libs/ardour/ardour/midi_region.h b/libs/ardour/ardour/midi_region.h
index 0ad60a900c..d3a6cbbbb8 100644
--- a/libs/ardour/ardour/midi_region.h
+++ b/libs/ardour/ardour/midi_region.h
@@ -45,6 +45,7 @@ template<typename Time> class EventSink;
 namespace ARDOUR {
 
 class MidiChannelFilter;
+class MidiCursor;
 class MidiFilter;
 class MidiModel;
 class MidiSource;
@@ -77,6 +78,7 @@ class LIBARDOUR_API MidiRegion : public Region
 	                    framepos_t position,
 	                    framecnt_t dur,
 	                    Evoral::Range<framepos_t>* loop_range,
+	                    MidiCursor& cursor,
 	                    uint32_t  chan_n = 0,
 	                    NoteMode  mode = Sustained,
 	                    MidiStateTracker* tracker = 0,
@@ -86,6 +88,7 @@ class LIBARDOUR_API MidiRegion : public Region
 	                           framepos_t position,
 	                           framecnt_t dur,
 	                           Evoral::Range<framepos_t>* loop_range,
+	                           MidiCursor& cursor,
 	                           uint32_t  chan_n = 0,
 	                           NoteMode  mode = Sustained) const;
 
@@ -130,6 +133,7 @@ class LIBARDOUR_API MidiRegion : public Region
 	                     framepos_t position,
 	                     framecnt_t dur,
 	                     Evoral::Range<framepos_t>* loop_range,
+	                     MidiCursor& cursor,
 	                     uint32_t chan_n = 0,
 	                     NoteMode mode = Sustained,
 	                     MidiStateTracker* tracker = 0,
diff --git a/libs/ardour/ardour/midi_source.h b/libs/ardour/ardour/midi_source.h
index 4815263739..1ff0a08f71 100644
--- a/libs/ardour/ardour/midi_source.h
+++ b/libs/ardour/ardour/midi_source.h
@@ -36,8 +36,9 @@
 namespace ARDOUR {
 
 class MidiChannelFilter;
-class MidiStateTracker;
+class MidiCursor;
 class MidiModel;
+class MidiStateTracker;
 
 template<typename T> class MidiRingBuffer;
 
@@ -87,18 +88,18 @@ class LIBARDOUR_API MidiSource : virtual public Source, public boost::enable_sha
 	 * \param tracker an optional pointer to MidiStateTracker object, for note on/off tracking.
 	 * \param filtered Parameters whose MIDI messages will not be returned.
 	 */
-
 	virtual framecnt_t midi_read (const Lock&                        lock,
 	                              Evoral::EventSink<framepos_t>&     dst,
 	                              framepos_t                         source_start,
 	                              framepos_t                         start,
 	                              framecnt_t                         cnt,
 	                              Evoral::Range<framepos_t>*         loop_range,
+	                              MidiCursor&                        cursor,
 	                              MidiStateTracker*                  tracker,
 	                              MidiChannelFilter*                 filter,
 	                              const std::set<Evoral::Parameter>& filtered,
-				      const double                       pulse,
-				      const double                       start_beats) const;
+	                              const double                       pulse,
+	                              const double                       start_beats) const;
 
 	/** Write data from a MidiRingBuffer to this source.
 	 *  @param source Source to read from.
@@ -175,10 +176,11 @@ class LIBARDOUR_API MidiSource : virtual public Source, public boost::enable_sha
 
 	/** Reset cached information (like iterators) when things have changed.
 	 * @param lock Source lock, which must be held by caller.
-	 * @param notes If non-NULL, currently active notes are added to this set.
 	 */
-	void invalidate(const Glib::Threads::Mutex::Lock&                       lock,
-	                std::set<Evoral::Sequence<Evoral::Beats>::WeakNotePtr>* notes=NULL);
+	void invalidate(const Glib::Threads::Mutex::Lock& lock);
+
+	/** Thou shalt not emit this directly, use invalidate() instead. */
+	mutable PBD::Signal1<void, bool> Invalidated;
 
 	void set_note_mode(const Glib::Threads::Mutex::Lock& lock, NoteMode mode);
 
@@ -230,11 +232,7 @@ class LIBARDOUR_API MidiSource : virtual public Source, public boost::enable_sha
 	boost::shared_ptr<MidiModel> _model;
 	bool                         _writing;
 
-	mutable Evoral::Sequence<Evoral::Beats>::const_iterator _model_iter;
-	mutable bool                                            _model_iter_valid;
-
-	mutable Evoral::Beats _length_beats;
-	mutable framepos_t    _last_read_end;
+	Evoral::Beats _length_beats;
 
 	/** The total duration of the current capture. */
 	framepos_t _capture_length;
diff --git a/libs/ardour/ardour/playlist.h b/libs/ardour/ardour/playlist.h
index 04615acb26..1530eede9f 100644
--- a/libs/ardour/ardour/playlist.h
+++ b/libs/ardour/ardour/playlist.h
@@ -276,6 +276,7 @@ public:
 	RegionListProperty   regions;  /* the current list of regions in the playlist */
 	std::set<boost::shared_ptr<Region> > all_regions; /* all regions ever added to this playlist */
 	PBD::ScopedConnectionList region_state_changed_connections;
+	PBD::ScopedConnectionList region_drop_references_connections;
 	DataType        _type;
 	int             _sort_id;
 	mutable gint    block_notifications;
@@ -359,6 +360,7 @@ public:
 
 
 	virtual void remove_dependents (boost::shared_ptr<Region> /*region*/) {}
+	virtual void region_going_away (boost::weak_ptr<Region> /*region*/) {}
 
 	virtual XMLNode& state (bool);
 
diff --git a/libs/ardour/midi_model.cc b/libs/ardour/midi_model.cc
index c94fb5ec17..80bc38cb0b 100644
--- a/libs/ardour/midi_model.cc
+++ b/libs/ardour/midi_model.cc
@@ -1455,8 +1455,7 @@ MidiModel::sync_to_source (const Glib::Threads::Mutex::Lock& source_lock)
 
 	/* Invalidate and store active notes, which will be picked up by the iterator
 	   on the next roll if time progresses linearly. */
-	ms->invalidate(source_lock,
-	               ms->session().transport_rolling() ? &_active_notes : NULL);
+	ms->invalidate(source_lock);
 
 	ms->mark_streaming_midi_write_started (source_lock, note_mode());
 
@@ -1625,8 +1624,7 @@ MidiModel::edit_lock()
 		   Add currently active notes to _active_notes so we can restore them
 		   if playback resumes at the same point after the edit. */
 		source_lock = new Glib::Threads::Mutex::Lock(ms->mutex());
-		ms->invalidate(*source_lock,
-		               ms->session().transport_rolling() ? &_active_notes : NULL);
+		ms->invalidate(*source_lock);
 	}
 
 	return WriteLock(new WriteLockImpl(source_lock, _lock, _control_lock));
diff --git a/libs/ardour/midi_playlist.cc b/libs/ardour/midi_playlist.cc
index b845758f47..4c200f60b5 100644
--- a/libs/ardour/midi_playlist.cc
+++ b/libs/ardour/midi_playlist.cc
@@ -195,7 +195,7 @@ MidiPlaylist::read (Evoral::EventSink<framepos_t>& dst,
 		                                                    mr->name(), start, dur, 
 		                                                    (loop_range ? loop_range->from : -1),
 		                                                    (loop_range ? loop_range->to : -1)));
-		mr->read_at (tgt, start, dur, loop_range, chan_n, _note_mode, &tracker->tracker, filter);
+		mr->read_at (tgt, start, dur, loop_range, tracker->cursor, chan_n, _note_mode, &tracker->tracker, filter);
 		DEBUG_TRACE (DEBUG::MidiPlaylistIO,
 		             string_compose ("\tPost-read: %1 active notes\n", tracker->tracker.on()));
 
@@ -208,6 +208,7 @@ MidiPlaylist::read (Evoral::EventSink<framepos_t>& dst,
 			                             mr->name(), ((new_tracker) ? "new" : "old")));
 
 			tracker->tracker.resolve_notes (tgt, loop_range ? loop_range->squish ((*i)->last_frame()) : (*i)->last_frame());
+			tracker->cursor.invalidate (false);
 			if (!new_tracker) {
 				_note_trackers.erase (t);
 			}
@@ -261,7 +262,7 @@ MidiPlaylist::region_edited(boost::shared_ptr<Region>         region,
 	/* Queue any necessary edit compensation events. */
 	t->second->fixer.prepare(
 		_session.tempo_map(), cmd, mr->position() - mr->start(),
-		_read_end, mr->midi_source()->model()->active_notes());
+		_read_end, t->second->cursor.active_notes);
 }
 
 void
@@ -292,6 +293,15 @@ MidiPlaylist::remove_dependents (boost::shared_ptr<Region> region)
 	_note_trackers.erase(region.get());
 }
 
+void
+MidiPlaylist::region_going_away (boost::weak_ptr<Region> region)
+{
+	boost::shared_ptr<Region> r = region.lock();
+	if (r) {
+		remove_dependents(r);
+	}
+}
+
 int
 MidiPlaylist::set_state (const XMLNode& node, int version)
 {
@@ -357,8 +367,12 @@ MidiPlaylist::destroy_region (boost::shared_ptr<Region> region)
 
 			i = tmp;
 		}
-	}
 
+		NoteTrackers::iterator t = _note_trackers.find(region.get());
+		if (t != _note_trackers.end()) {
+			_note_trackers.erase(t);
+		}
+	}
 
 	if (changed) {
 		/* overload this, it normally means "removed", not destroyed */
diff --git a/libs/ardour/midi_region.cc b/libs/ardour/midi_region.cc
index f5c9df1b6e..66c1cf6eef 100644
--- a/libs/ardour/midi_region.cc
+++ b/libs/ardour/midi_region.cc
@@ -336,12 +336,13 @@ MidiRegion::read_at (Evoral::EventSink<framepos_t>& out,
                      framepos_t                     position,
                      framecnt_t                     dur,
                      Evoral::Range<framepos_t>*     loop_range,
+                     MidiCursor&                    cursor,
                      uint32_t                       chan_n,
                      NoteMode                       mode,
                      MidiStateTracker*              tracker,
                      MidiChannelFilter*             filter) const
 {
-	return _read_at (_sources, out, position, dur, loop_range, chan_n, mode, tracker, filter);
+	return _read_at (_sources, out, position, dur, loop_range, cursor, chan_n, mode, tracker, filter);
 }
 
 framecnt_t
@@ -349,10 +350,11 @@ MidiRegion::master_read_at (MidiRingBuffer<framepos_t>& out,
                             framepos_t                  position,
                             framecnt_t                  dur,
                             Evoral::Range<framepos_t>*  loop_range,
+                            MidiCursor&                 cursor,
                             uint32_t                    chan_n,
                             NoteMode                    mode) const
 {
-	return _read_at (_master_sources, out, position, dur, loop_range, chan_n, mode); /* no tracker */
+	return _read_at (_master_sources, out, position, dur, loop_range, cursor, chan_n, mode); /* no tracker */
 }
 
 framecnt_t
@@ -361,6 +363,7 @@ MidiRegion::_read_at (const SourceList&              /*srcs*/,
                       framepos_t                     position,
                       framecnt_t                     dur,
                       Evoral::Range<framepos_t>*     loop_range,
+                      MidiCursor&                    cursor,
                       uint32_t                       chan_n,
                       NoteMode                       mode,
                       MidiStateTracker*              tracker,
@@ -423,6 +426,7 @@ MidiRegion::_read_at (const SourceList&              /*srcs*/,
 		    _start + internal_offset, // where to start reading in the source
 		    to_read, // read duration in frames
 		    loop_range,
+		    cursor,
 		    tracker,
 		    filter,
 		    _filtered_parameters,
diff --git a/libs/ardour/midi_source.cc b/libs/ardour/midi_source.cc
index d675662a6a..edfc27acb9 100644
--- a/libs/ardour/midi_source.cc
+++ b/libs/ardour/midi_source.cc
@@ -40,13 +40,14 @@
 #include "ardour/debug.h"
 #include "ardour/file_source.h"
 #include "ardour/midi_channel_filter.h"
+#include "ardour/midi_cursor.h"
 #include "ardour/midi_model.h"
 #include "ardour/midi_source.h"
 #include "ardour/midi_state_tracker.h"
 #include "ardour/session.h"
-#include "ardour/tempo.h"
 #include "ardour/session_directory.h"
 #include "ardour/source_factory.h"
+#include "ardour/tempo.h"
 
 #include "pbd/i18n.h"
 
@@ -59,9 +60,7 @@ using namespace PBD;
 MidiSource::MidiSource (Session& s, string name, Source::Flag flags)
 	: Source(s, DataType::MIDI, name, flags)
 	, _writing(false)
-	, _model_iter_valid(false)
 	, _length_beats(0.0)
-	, _last_read_end(0)
 	, _capture_length(0)
 	, _capture_loop_length(0)
 {
@@ -70,9 +69,7 @@ MidiSource::MidiSource (Session& s, string name, Source::Flag flags)
 MidiSource::MidiSource (Session& s, const XMLNode& node)
 	: Source(s, node)
 	, _writing(false)
-	, _model_iter_valid(false)
 	, _length_beats(0.0)
-	, _last_read_end(0)
 	, _capture_length(0)
 	, _capture_loop_length(0)
 {
@@ -177,10 +174,9 @@ MidiSource::update_length (framecnt_t)
 }
 
 void
-MidiSource::invalidate (const Lock& lock, std::set<Evoral::Sequence<Evoral::Beats>::WeakNotePtr>* notes)
+MidiSource::invalidate (const Lock& lock)
 {
-	_model_iter_valid = false;
-	_model_iter.invalidate(notes);
+	Invalidated(_session.transport_rolling());
 }
 
 framecnt_t
@@ -190,13 +186,14 @@ MidiSource::midi_read (const Lock&                        lm,
                        framepos_t                         start,
                        framecnt_t                         cnt,
                        Evoral::Range<framepos_t>*         loop_range,
+                       MidiCursor&                        cursor,
                        MidiStateTracker*                  tracker,
                        MidiChannelFilter*                 filter,
                        const std::set<Evoral::Parameter>& filtered,
-		       const double                       pulse,
-		       const double                       start_beats) const
+                       const double                       pulse,
+                       const double                       start_beats) const
 {
-	//BeatsFramesConverter converter(_session.tempo_map(), source_start);
+	BeatsFramesConverter converter(_session.tempo_map(), source_start);
 
 	const double start_qn = (pulse * 4.0) - start_beats;
 
@@ -209,63 +206,21 @@ MidiSource::midi_read (const Lock&                        lm,
 	}
 
 	// Find appropriate model iterator
-	Evoral::Sequence<Evoral::Beats>::const_iterator& i = _model_iter;
-	const bool linear_read = _last_read_end != 0 && start == _last_read_end;
-	if (!linear_read || !_model_iter_valid) {
-#if 0
-		// Cached iterator is invalid, search for the first event past start
-		i = _model->begin(converter.from(start), false, filtered,
-		                  linear_read ? &_model->active_notes() : NULL);
-		_model_iter_valid = true;
-		if (!linear_read) {
-			_model->active_notes().clear();
-		}
-#else
-		/* hot-fix http://tracker.ardour.org/view.php?id=6541
-		 * "parallel playback of linked midi regions -> no note-offs"
-		 *
-		 * A midi source can be used by multiple tracks simultaneously,
-		 * in which case midi_read() may be called from different tracks for
-		 * overlapping time-ranges.
-		 *
-		 * However there is only a single iterator for a given midi-source.
-		 * This results in every midi_read() performing a seek.
-		 *
-		 * If seeking is performed with
-		 *    _model->begin(converter.from(start),...)
-		 * the model is used for seeking. That method seeks to the first
-		 * *note-on* event after 'start'.
-		 *
-		 * _model->begin(converter.from(  ) ,..) eventually calls
-		 * Sequence<Time>::const_iterator() in libs/evoral/src/Sequence.cpp
-		 * which looks up the note-event via seq.note_lower_bound(t);
-		 * but the sequence 'seq' only contains note-on events(!).
-		 * note-off events are implicit in Sequence<Time>::operator++()
-		 * via _active_notes.pop(); and not part of seq.
-		 *
-		 * see also http://tracker.ardour.org/view.php?id=6287#c16671
-		 *
-		 * The linear search below assures that reading starts at the first
-		 * event for the given time, regardless of its event-type.
-		 *
-		 * The performance of this approach is O(N), while the previous
-		 * implementation is O(log(N)). This needs to be optimized:
-		 * The model-iterator or event-sequence needs to be re-designed in
-		 * some way (maybe keep an iterator per playlist).
-		 */
-		for (i = _model->begin(); i != _model->end(); ++i) {
-			if (i->time().to_double() >= start_beats) {
-				break;
-			}
-		}
-		_model_iter_valid = true;
-		if (!linear_read) {
-			_model->active_notes().clear();
-		}
-#endif
+	Evoral::Sequence<Evoral::Beats>::const_iterator& i = cursor.iter;
+	const bool linear_read = cursor.last_read_end != 0 && start == cursor.last_read_end;
+	if (!linear_read || !i.valid()) {
+		/* Cached iterator is invalid, search for the first event past start.
+		   Note that multiple tracks can use a MidiSource simultaneously, so
+		   all playback state must be in parameters (the cursor) and must not
+		   be cached in the source of model itself.
+		   See http://tracker.ardour.org/view.php?id=6541
+		*/
+		cursor.connect(Invalidated);
+		cursor.iter = _model->begin(converter.from(start), false, filtered, &cursor.active_notes);
+		cursor.active_notes.clear();
 	}
 
-	_last_read_end = start + cnt;
+	cursor.last_read_end = start + cnt;
 
 	// Copy events in [start, start + cnt) into dst
 	for (; i != _model->end(); ++i) {
@@ -338,7 +293,6 @@ MidiSource::midi_write (const Lock&                 lm,
 	const framecnt_t ret = write_unlocked (lm, source, source_start, cnt);
 
 	if (cnt == max_framecnt) {
-		_last_read_end = 0;
 		invalidate(lm);
 	} else {
 		_capture_length += cnt;
diff --git a/libs/ardour/playlist.cc b/libs/ardour/playlist.cc
index 70744429f9..fcf24795db 100644
--- a/libs/ardour/playlist.cc
+++ b/libs/ardour/playlist.cc
@@ -765,6 +765,7 @@ Playlist::flush_notifications (bool from_undo)
 	 notify_region_added (region);
 
 	 region->PropertyChanged.connect_same_thread (region_state_changed_connections, boost::bind (&Playlist::region_changed_proxy, this, _1, boost::weak_ptr<Region> (region)));
+	 region->DropReferences.connect_same_thread (region_drop_references_connections, boost::bind (&Playlist::region_going_away, this, boost::weak_ptr<Region> (region)));
 
 	 return true;
  }
@@ -1745,6 +1746,7 @@ Playlist::region_bounds_changed (const PropertyChange& what_changed, boost::shar
 		 RegionWriteLock rl (this);
 
 		 region_state_changed_connections.drop_connections ();
+		 region_drop_references_connections.drop_connections ();
 
 		 for (RegionList::iterator i = regions.begin(); i != regions.end(); ++i) {
 			 pending_removes.insert (*i);