Fix deadlock and potential race condition when editing MIDI.

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

libs/ardour/ardour/midi_source.h

@@ -85,6 +85,11 @@ class MidiSource : virtual public Source
 	virtual void load_model(bool lock=true, bool force_reload=false) = 0;
 	virtual void destroy_model() = 0;
 
+	/** This must be called with the source lock held whenever the
+	 *  source/model contents have been changed (reset iterators/cache/etc).
+	 */
+	void invalidate();
+
 	void set_note_mode(NoteMode mode);
 	
 	void set_timeline_position (int64_t pos);

libs/ardour/ardour/source.h

@@ -116,6 +116,7 @@ class Source : public SessionObject, public ARDOUR::Readable, public boost::nonc
 		return Evoral::IdentityConverter<double, nframes_t>();
 	}
 
+	Glib::Mutex& mutex()       { return _lock; }
 	Flag         flags() const { return _flags; }
 
   protected:

libs/ardour/midi_model.cc

@@ -1,6 +1,6 @@
 /*
     Copyright (C) 2007 Paul Davis
- Written by Dave Robillard, 2007
+    Author: Dave 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
@@ -41,7 +41,6 @@ MidiModel::MidiModel(MidiSource *s, size_t size)
 	: AutomatableSequence<TimeType>(s->session(), size)
 	, _midi_source(s)
 {
-	//cerr << "MidiModel \"" << s->name() << "\" constructed: " << this << endl;
 }
 
 /** Start a new command.
@@ -50,7 +49,8 @@ MidiModel::MidiModel(MidiSource *s, size_t size)
  * can be held on to for as long as the caller wishes, or discarded without
  * formality, until apply_command is called and ownership is taken.
  */
-MidiModel::DeltaCommand* MidiModel::new_delta_command(const string name)
+MidiModel::DeltaCommand*
+MidiModel::new_delta_command(const string name)
 {
 	DeltaCommand* cmd = new DeltaCommand(_midi_source->model(), name);
 	return cmd;
@@ -73,16 +73,14 @@ MidiModel::apply_command(Session& session, Command* cmd)
 
 // DeltaCommand
 
-MidiModel::DeltaCommand::DeltaCommand(boost::shared_ptr<MidiModel> m,
-		const std::string& name)
+MidiModel::DeltaCommand::DeltaCommand(boost::shared_ptr<MidiModel> m, const std::string& name)
 	: Command(name)
 	, _model(m)
 	, _name(name)
 {
 }
 
-MidiModel::DeltaCommand::DeltaCommand(boost::shared_ptr<MidiModel> m,
-		const XMLNode& node)
+MidiModel::DeltaCommand::DeltaCommand(boost::shared_ptr<MidiModel> m, const XMLNode& node)
 	: _model(m)
 {
 	set_state(node);
@@ -91,7 +89,6 @@ MidiModel::DeltaCommand::DeltaCommand(boost::shared_ptr<MidiModel> m,
 void
 MidiModel::DeltaCommand::add(const boost::shared_ptr< Evoral::Note<TimeType> > note)
 {
-	//cerr << "MEC: apply" << endl;
 	_removed_notes.remove(note);
 	_added_notes.push_back(note);
 }
@@ -99,7 +96,6 @@ MidiModel::DeltaCommand::add(const boost::shared_ptr< Evoral::Note<TimeType> > n
 void
 MidiModel::DeltaCommand::remove(const boost::shared_ptr< Evoral::Note<TimeType> > note)
 {
-	//cerr << "MEC: remove" << endl;
 	_added_notes.remove(note);
 	_removed_notes.push_back(note);
 }
@@ -110,12 +106,10 @@ 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();
 
-	// Store the current seek position so we can restore the read iterator
-	// after modifying the contents of the model
-	const TimeType read_time = _model->read_time();
-
 	for (NoteList::iterator i = _added_notes.begin(); i != _added_notes.end(); ++i)
 		_model->add_note_unlocked(*i);
 
@@ -123,9 +117,6 @@ MidiModel::DeltaCommand::operator()()
 		_model->remove_note_unlocked(*i);
 
 	_model->write_unlock();
-	// FIXME: race?
-	_model->read_seek(read_time); // restore read position
-
 	_model->ContentsChanged(); /* EMIT SIGNAL */
 }
 
@@ -135,12 +126,10 @@ 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();
 
-	// Store the current seek position so we can restore the read iterator
-	// after modifying the contents of the model
-	const TimeType read_time = _model->read_time();
-
 	for (NoteList::iterator i = _added_notes.begin(); i != _added_notes.end(); ++i)
 		_model->remove_note_unlocked(*i);
 
@@ -148,9 +137,6 @@ MidiModel::DeltaCommand::undo()
 		_model->add_note_unlocked(*i);
 
 	_model->write_unlock();
-	// FIXME: race?
-	_model->read_seek(read_time); // restore read position
-
 	_model->ContentsChanged(); /* EMIT SIGNAL */
 }
 

libs/ardour/midi_source.cc

@@ -103,6 +103,12 @@ MidiSource::set_state (const XMLNode& node)
 	return 0;
 }
 
+void
+MidiSource::invalidate ()
+{
+	_model_iter.invalidate();
+}
+
 nframes_t
 MidiSource::midi_read (MidiRingBuffer<nframes_t>& dst, nframes_t start, nframes_t cnt,
 		nframes_t stamp_offset, nframes_t negative_stamp_offset) const
@@ -114,7 +120,7 @@ MidiSource::midi_read (MidiRingBuffer<nframes_t>& dst, nframes_t start, nframes_
 
 		Evoral::Sequence<double>::const_iterator& i = _model_iter;
 		
-		if (_last_read_end == 0 || start != _last_read_end) {
+		if (_last_read_end == 0 || start != _last_read_end || !i.valid()) {
 			cerr << "MidiSource::midi_read seeking to frame " << start << endl;
 			for (i = _model->begin(); i != _model->end(); ++i) {
 				if (BEATS_TO_FRAMES(i->time()) >= start) {

libs/evoral/evoral/Sequence.hpp

@@ -63,8 +63,6 @@ class Sequence : virtual public ControlSet {
 public:
 	Sequence(const TypeMap& type_map, size_t size=0);
 	
-	bool read_locked() { return _read_iter.locked(); }
-
 	void write_lock();
 	void write_unlock();
 
@@ -127,6 +125,8 @@ public:
 		inline bool valid() const { return !_is_end && _event; }
 		inline bool locked() const { return _locked; }
 		
+		void invalidate();
+
 		const Event<Time>& operator*()  const { return *_event;  }
 		const boost::shared_ptr< Event<Time> > operator->() const  { return _event; }
 		const boost::shared_ptr< Event<Time> > get_event_pointer() { return _event; }
@@ -159,9 +159,6 @@ public:
 	const_iterator        begin(Time t=0) const { return const_iterator(*this, t); }
 	const const_iterator& end()           const { return _end_iter; }
 	
-	void read_seek(Time t) { _read_iter = begin(t); }
-	Time read_time() const { return _read_iter.valid() ? _read_iter->time() : 0.0; }
-
 	bool control_to_midi_event(boost::shared_ptr< Event<Time> >& ev,
 	                           const ControlIterator&            iter) const;
 	
@@ -175,7 +172,6 @@ public:
 	uint8_t highest_note() const { return _highest_note; }
 	
 protected:
-	mutable const_iterator _read_iter;
 	bool _edited;
 
 private:

libs/evoral/src/Sequence.cpp

@@ -224,6 +224,26 @@ Sequence<Time>::const_iterator::~const_iterator()
 	}
 }
 
+template<typename Time>
+void
+Sequence<Time>::const_iterator::invalidate()
+{
+	while (!_active_notes.empty()) {
+		_active_notes.pop();
+	}
+	_type = NIL;
+	_is_end = true;
+	if (_seq) {
+		_note_iter = _seq->notes().end();
+		_sysex_iter = _seq->sysexes().end();
+	}
+	_control_iter = _control_iters.end();
+	if (_locked) {
+		_seq->read_unlock();
+		_locked = false;
+	}
+}
+
 template<typename Time>
 const typename Sequence<Time>::const_iterator&
 Sequence<Time>::const_iterator::operator++()
@@ -407,8 +427,7 @@ Sequence<Time>::const_iterator::operator=(const const_iterator& other)
 
 template<typename Time>
 Sequence<Time>::Sequence(const TypeMap& type_map, size_t size)
-	: _read_iter(*this, DBL_MAX)
-	, _edited(false)
+	: _edited(false)
 	, _type_map(type_map)
 	, _notes(size)
 	, _writing(false)
@@ -508,7 +527,6 @@ Sequence<Time>::clear()
 	_notes.clear();
 	for (Controls::iterator li = _controls.begin(); li != _controls.end(); ++li)
 		li->second->list()->clear();
-	_read_iter = end();
 	_lock.writer_unlock();
 }