Replace horribly error-prone Sequence/MidiModel/MidiSource locking API with scoped locks that automatically Do The Right Thing.

Make Sequence::read_lock const correct in the process (a read lock can be taken out on a const Sequence, but not a write lock).


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

4 files changed, 39 insertions, 24 deletions

diff --git a/libs/ardour/ardour/midi_model.h b/libs/ardour/ardour/midi_model.h
index 6496fd169a..cebc435435 100644
--- a/libs/ardour/ardour/midi_model.h
+++ b/libs/ardour/ardour/midi_model.h
@@ -166,6 +166,21 @@ public:
 	boost::shared_ptr<Evoral::Note<TimeType> > find_note (boost::shared_ptr<Evoral::Note<TimeType> >);
 
 private:
+	struct WriteLockImpl : public AutomatableSequence<Evoral::MusicalTime>::WriteLockImpl {
+		WriteLockImpl(Glib::Mutex::Lock* source_lock, Glib::RWLock& s, Glib::Mutex& c)
+			: AutomatableSequence<Evoral::MusicalTime>::WriteLockImpl(s, c)
+			, source_lock(source_lock)
+		{}
+		~WriteLockImpl() {
+			delete source_lock;
+		}
+		Glib::Mutex::Lock* source_lock;
+	};
+
+public:
+	virtual WriteLock write_lock();
+
+private:
 	friend class DeltaCommand;
 
 	// We cannot use a boost::shared_ptr here to avoid a retain cycle
diff --git a/libs/ardour/ardour/midi_source.h b/libs/ardour/ardour/midi_source.h
index 0192d94e2d..1b452a7c4d 100644
--- a/libs/ardour/ardour/midi_source.h
+++ b/libs/ardour/ardour/midi_source.h
@@ -132,7 +132,7 @@ class MidiSource : virtual public Source
 	bool                         _writing;
 
 	mutable Evoral::Sequence<Evoral::MusicalTime>::const_iterator _model_iter;
-	mutable bool                                                  _model_iterator_valid;
+	mutable bool                                                  _model_iter_valid;
 
 	mutable double    _length_beats;
 	mutable sframes_t _last_read_end;
diff --git a/libs/ardour/midi_model.cc b/libs/ardour/midi_model.cc
index e35c5c6d98..2b63533537 100644
--- a/libs/ardour/midi_model.cc
+++ b/libs/ardour/midi_model.cc
@@ -135,9 +135,7 @@ MidiModel::DeltaCommand::operator()()
 	// This could be made much faster by using a priority_queue for added and
 	// removed notes (or sort here), and doing a single iteration over _model
 
-	Glib::Mutex::Lock lm (_model->_midi_source->mutex());
-	_model->_midi_source->invalidate(); // release model read lock
-	_model->write_lock();
+	MidiModel::WriteLock lock(_model->write_lock());
 
 	for (NoteList::iterator i = _added_notes.begin(); i != _added_notes.end(); ++i) {
 		_model->add_note_unlocked(*i);
@@ -147,7 +145,7 @@ MidiModel::DeltaCommand::operator()()
 		_model->remove_note_unlocked(*i);
 	}
 
-	_model->write_unlock();
+	lock.reset();
 	_model->ContentsChanged(); /* EMIT SIGNAL */
 }
 
@@ -157,9 +155,7 @@ MidiModel::DeltaCommand::undo()
 	// This could be made much faster by using a priority_queue for added and
 	// removed notes (or sort here), and doing a single iteration over _model
 
-	Glib::Mutex::Lock lm (_model->_midi_source->mutex());
-	_model->_midi_source->invalidate(); // release model read lock
-	_model->write_lock();
+	MidiModel::WriteLock lock(_model->write_lock());;
 
 	for (NoteList::iterator i = _added_notes.begin(); i != _added_notes.end(); ++i) {
 		_model->remove_note_unlocked(*i);
@@ -169,7 +165,7 @@ MidiModel::DeltaCommand::undo()
 		_model->add_note_unlocked(*i);
 	}
 
-	_model->write_unlock();
+	lock.reset();
 	_model->ContentsChanged(); /* EMIT SIGNAL */
 }
 
@@ -387,9 +383,7 @@ MidiModel::DiffCommand::change(const boost::shared_ptr< Evoral::Note<TimeType> >
 void
 MidiModel::DiffCommand::operator()()
 {
-	Glib::Mutex::Lock lm (_model->_midi_source->mutex());
-	_model->_midi_source->invalidate(); // release model read lock
-	_model->write_lock();
+	MidiModel::WriteLock lock(_model->write_lock());
 
 	for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
 		Property prop = i->property;
@@ -412,16 +406,14 @@ MidiModel::DiffCommand::operator()()
 		}
 	}
 
-	_model->write_unlock();
+	lock.reset();
 	_model->ContentsChanged(); /* EMIT SIGNAL */
 }
 
 void
 MidiModel::DiffCommand::undo()
 {
-	Glib::Mutex::Lock lm (_model->_midi_source->mutex());
-	_model->_midi_source->invalidate(); // release model read lock
-	_model->write_lock();
+	MidiModel::WriteLock lock(_model->write_lock());
 
 	for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
 		Property prop = i->property;
@@ -444,7 +436,7 @@ MidiModel::DiffCommand::undo()
 		}
 	}
 
-	_model->write_unlock();
+	lock.reset();
 	_model->ContentsChanged(); /* EMIT SIGNAL */
 }
 
@@ -690,7 +682,7 @@ MidiModel::DiffCommand::get_state ()
 bool
 MidiModel::write_to(boost::shared_ptr<MidiSource> source)
 {
-	read_lock();
+	ReadLock lock(read_lock());
 
 	const bool old_percussive = percussive();
 	set_percussive(false);
@@ -703,7 +695,6 @@ MidiModel::write_to(boost::shared_ptr<MidiSource> source)
 
 	set_percussive(old_percussive);
 
-	read_unlock();
 	set_edited(false);
 
 	return true;
@@ -731,3 +722,11 @@ MidiModel::find_note (boost::shared_ptr<Evoral::Note<TimeType> > other)
 
 	return boost::shared_ptr<Evoral::Note<TimeType> >();
 }
+
+MidiModel::WriteLock
+MidiModel::write_lock()
+{
+	Glib::Mutex::Lock* source_lock = new Glib::Mutex::Lock(_midi_source->mutex());
+	_midi_source->invalidate(); // Release cached iterator's read lock on model
+	return WriteLock(new WriteLockImpl(source_lock, _lock, _control_lock));
+}
diff --git a/libs/ardour/midi_source.cc b/libs/ardour/midi_source.cc
index 86f0b86f95..133b75893d 100644
--- a/libs/ardour/midi_source.cc
+++ b/libs/ardour/midi_source.cc
@@ -55,7 +55,7 @@ MidiSource::MidiSource (Session& s, string name, Source::Flag flags)
 	, _read_data_count(0)
 	, _write_data_count(0)
 	, _writing(false)
-	, _model_iterator_valid(true)
+	, _model_iter_valid(false)
 	, _length_beats(0.0)
 	, _last_read_end(0)
 	, _last_write_end(0)
@@ -67,7 +67,7 @@ MidiSource::MidiSource (Session& s, const XMLNode& node)
 	, _read_data_count(0)
 	, _write_data_count(0)
 	, _writing(false)
-	, _model_iterator_valid(true)
+	, _model_iter_valid(false)
 	, _length_beats(0.0)
 	, _last_read_end(0)
 	, _last_write_end(0)
@@ -124,7 +124,8 @@ MidiSource::update_length (sframes_t /*pos*/, sframes_t /*cnt*/)
 void
 MidiSource::invalidate ()
 {
-	_model_iterator_valid = false;
+	_model_iter_valid = false;
+	_model_iter.invalidate();
 }
 
 nframes_t
@@ -144,13 +145,13 @@ MidiSource::midi_read (MidiRingBuffer<nframes_t>& dst, sframes_t source_start,
 		Evoral::Sequence<double>::const_iterator& i = _model_iter;
 
 		// If the cached iterator is invalid, search for the first event past start
-		if (_last_read_end == 0 || start != _last_read_end || !_model_iterator_valid) {
+		if (_last_read_end == 0 || start != _last_read_end || !_model_iter_valid) {
 			for (i = _model->begin(); i != _model->end(); ++i) {
 				if (BEATS_TO_FRAMES(i->time()) >= start) {
 					break;
 				}
 			}
-			_model_iterator_valid = true;
+			_model_iter_valid = true;
 		}
 
 		_last_read_end = start + cnt;