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

gtk2_ardour/midi_region_view.cc

@@ -772,7 +772,7 @@ MidiRegionView::redisplay_model()
 		(*i)->invalidate ();
 	}
 
-	_model->read_lock();
+	MidiModel::ReadLock lock(_model->read_lock());
 
 	MidiModel::Notes& notes (_model->notes());
 	_optimization_iterator = _events.begin();
@@ -832,8 +832,6 @@ MidiRegionView::redisplay_model()
 	display_sysexes();
 	display_program_changes();
 
-	_model->read_unlock();
-
 	_marked_for_selection.clear ();
 	_marked_for_velocity.clear ();
 

libs/ardour/ardour/midi_model.h

@@ -165,6 +165,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;
 

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;

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());
+	MidiModel::WriteLock lock(_model->write_lock());
-	_model->_midi_source->invalidate(); // release model read 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());
+	MidiModel::WriteLock lock(_model->write_lock());;
-	_model->_midi_source->invalidate(); // release model read 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());
+	MidiModel::WriteLock lock(_model->write_lock());
-	_model->_midi_source->invalidate(); // release model read 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());
+	MidiModel::WriteLock lock(_model->write_lock());
-	_model->_midi_source->invalidate(); // release model read 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));
+}

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;

libs/evoral/evoral/Sequence.hpp

@@ -63,11 +63,26 @@ class Sequence : virtual public ControlSet {
 public:
 	Sequence(const TypeMap& type_map);
 
-	void write_lock();
+protected:
-	void write_unlock();
+	struct WriteLockImpl {
+		WriteLockImpl(Glib::RWLock& s, Glib::Mutex& c)
+			: sequence_lock(new Glib::RWLock::WriterLock(s))
+			, control_lock(new Glib::Mutex::Lock(c))
+		{ }
+		~WriteLockImpl() {
+			delete sequence_lock;
+			delete control_lock;
+		}
+		Glib::RWLock::WriterLock* sequence_lock;
+		Glib::Mutex::Lock*        control_lock;
+	};
 
-	void read_lock()   const;
+public:
-	void read_unlock() const;
+	typedef boost::shared_ptr<Glib::RWLock::ReaderLock> ReadLock;
+	typedef boost::shared_ptr<WriteLockImpl>            WriteLock;
+
+	virtual ReadLock  read_lock() const { return ReadLock(new Glib::RWLock::ReaderLock(_lock)); }
+	virtual WriteLock write_lock()      { return WriteLock(new WriteLockImpl(_lock, _control_lock)); }
 
 	void clear();
 
@@ -143,7 +158,7 @@ public:
 		~const_iterator();
 
 		inline bool valid() const { return !_is_end && _event; }
-		inline bool locked() const { return _locked; }
+		//inline bool locked() const { return _locked; }
 
 		void invalidate();
 
@@ -169,7 +184,7 @@ public:
 		mutable ActiveNotes              _active_notes;
 		MIDIMessageType                  _type;
 		bool                             _is_end;
-		bool                             _locked;
+		typename Sequence::ReadLock      _lock;
 		typename Notes::const_iterator   _note_iter;
 		typename SysExes::const_iterator _sysex_iter;
 		ControlIterators                 _control_iters;
@@ -194,7 +209,8 @@ public:
 	uint8_t highest_note() const { return _highest_note; }
 
 protected:
-	bool _edited;
+	bool                 _edited;
+	mutable Glib::RWLock _lock;
 
 private:
 	friend class const_iterator;
@@ -204,8 +220,6 @@ private:
 	void append_control_unlocked(const Parameter& param, Time time, double value);
 	void append_sysex_unlocked(const MIDIEvent<Time>& ev);
 
-	mutable Glib::RWLock _lock;
-
 	const TypeMap& _type_map;
 
 	Notes   _notes;

libs/evoral/src/Sequence.cpp

@@ -46,36 +46,12 @@ using namespace std;
 
 namespace Evoral {
 
-template<typename Time>
-void Sequence<Time>::write_lock() {
-	_lock.writer_lock();
-	_control_lock.lock();
-}
-
-template<typename Time>
-void Sequence<Time>::write_unlock() {
-	_lock.writer_unlock();
-	_control_lock.unlock();
-}
-
-template<typename Time>
-void Sequence<Time>::read_lock() const {
-	_lock.reader_lock();
-}
-
-template<typename Time>
-void Sequence<Time>::read_unlock() const {
-	_lock.reader_unlock();
-}
-
-
 // Read iterator (const_iterator)
 
 template<typename Time>
 Sequence<Time>::const_iterator::const_iterator()
 	: _seq(NULL)
 	, _is_end(true)
-	, _locked(false)
 	, _control_iter(_control_iters.end())
 {
 	_event = boost::shared_ptr< Event<Time> >(new Event<Time>());
@@ -86,18 +62,19 @@ Sequence<Time>::const_iterator::const_iterator(const Sequence<Time>& seq, Time t
 	: _seq(&seq)
 	, _type(NIL)
 	, _is_end((t == DBL_MAX) || seq.empty())
-	, _locked(!_is_end)
 	, _note_iter(seq.notes().end())
 	, _sysex_iter(seq.sysexes().end())
 	, _control_iter(_control_iters.end())
 {
 	DUMP(format("Created Iterator @ %1% (is end: %2%)\n)") % t % _is_end);
 
-	if (_is_end) {
+	if (!_is_end) {
+		_lock = seq.read_lock();
+	} else {
 		return;
 	}
 
-	seq.read_lock();
+	typename Sequence<Time>::ReadLock lock(seq.read_lock());
 
 	// Find first note which begins at or after t
 	_note_iter = seq.note_lower_bound(t);
@@ -199,8 +176,6 @@ Sequence<Time>::const_iterator::const_iterator(const Sequence<Time>& seq, Time t
 		DUMP(format("Starting at end @ %1%\n") % t);
 		_type   = NIL;
 		_is_end = true;
-		_locked = false;
-		_seq->read_unlock();
 	} else {
 		DUMP(printf("New iterator = 0x%x : 0x%x @ %f\n",
 			    (int)_event->event_type(),
@@ -213,9 +188,6 @@ Sequence<Time>::const_iterator::const_iterator(const Sequence<Time>& seq, Time t
 template<typename Time>
 Sequence<Time>::const_iterator::~const_iterator()
 {
-	if (_locked) {
-		_seq->read_unlock();
-	}
 }
 
 template<typename Time>
@@ -232,10 +204,7 @@ Sequence<Time>::const_iterator::invalidate()
 		_sysex_iter = _seq->sysexes().end();
 	}
 	_control_iter = _control_iters.end();
-	if (_locked) {
+	_lock.reset();
-		_seq->read_unlock();
-		_locked = false;
-	}
 }
 
 template<typename Time>
@@ -386,27 +355,20 @@ template<typename Time>
 typename Sequence<Time>::const_iterator&
 Sequence<Time>::const_iterator::operator=(const const_iterator& other)
 {
-	if (_seq != other._seq) {
-		if (_locked) {
-			_seq->read_unlock();
-		}
-		if (other._locked) {
-		   other._seq->read_lock();
-		}
-	} else if (!_locked && other._locked) {
-		_seq->read_lock();
-	}
-
 	_seq           = other._seq;
 	_event         = other._event;
 	_active_notes  = other._active_notes;
 	_type          = other._type;
 	_is_end        = other._is_end;
-	_locked        = other._locked;
 	_note_iter     = other._note_iter;
 	_sysex_iter    = other._sysex_iter;
 	_control_iters = other._control_iters;
 
+	if (other._lock)
+		_lock = _seq->read_lock();
+	else
+		_lock.reset();
+
 	if (other._control_iter == other._control_iters.end()) {
 		_control_iter = _control_iters.end();
 	} else {
@@ -431,7 +393,7 @@ Sequence<Time>::Sequence(const TypeMap& type_map)
 {
 	DUMP(format("Sequence (size %1%) constructed: %2%\n") % size % this);
 	assert(_end_iter._is_end);
-	assert( ! _end_iter._locked);
+	assert( ! _end_iter._lock);
 }
 
 /** Write the controller event pointed to by \a iter to \a ev.
@@ -516,11 +478,10 @@ template<typename Time>
 void
 Sequence<Time>::clear()
 {
-	_lock.writer_lock();
+	WriteLock lock(write_lock());
 	_notes.clear();
 	for (Controls::iterator li = _controls.begin(); li != _controls.end(); ++li)
 		li->second->list()->clear();
-	_lock.writer_unlock();
 }
 
 /** Begin a write of events to the model.
@@ -535,13 +496,12 @@ void
 Sequence<Time>::start_write()
 {
 	DUMP(format("%1% : start_write (percussive = %2%)\n") % this % _percussive);
-	write_lock();
+	WriteLock lock(write_lock());
 	_writing = true;
 	for (int i = 0; i < 16; ++i) {
 		_write_notes[i].clear();
 	}
 	_dirty_controls.clear();
-	write_unlock();
 }
 
 /** Finish a write of events to the model.
@@ -554,10 +514,9 @@ template<typename Time>
 void
 Sequence<Time>::end_write(bool delete_stuck)
 {
-	write_lock();
+	WriteLock lock(write_lock());
 
 	if (!_writing) {
-		write_unlock();
 		return;
 	}
 
@@ -588,7 +547,6 @@ Sequence<Time>::end_write(bool delete_stuck)
 	}
 
 	_writing = false;
-	write_unlock();
 }
 
 /** Append \a ev to model.  NOT realtime safe.
@@ -601,7 +559,7 @@ template<typename Time>
 void
 Sequence<Time>::append(const Event<Time>& event)
 {
-	write_lock();
+	WriteLock lock(write_lock());
 	_edited = true;
 
 	const MIDIEvent<Time>& ev = (const MIDIEvent<Time>&)event;
@@ -611,7 +569,6 @@ Sequence<Time>::append(const Event<Time>& event)
 
 	if (!midi_event_is_valid(ev.buffer(), ev.size())) {
 		cerr << "WARNING: Sequence ignoring illegal MIDI event" << endl;
-		write_unlock();
 		return;
 	}
 
@@ -647,8 +604,6 @@ Sequence<Time>::append(const Event<Time>& event)
 	} else {
 		printf("WARNING: Sequence: Unknown MIDI event type %X\n", ev.type());
 	}
-
-	write_unlock();
 }
 
 template<typename Time>