From d98c8e8fa4bb18cbbf2a18b82b11d9949bb00890 Mon Sep 17 00:00:00 2001 From: David Robillard Date: Thu, 22 Oct 2009 16:15:36 +0000 Subject: 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 | 4 +-- libs/ardour/ardour/midi_model.h | 15 ++++++++ libs/ardour/ardour/midi_source.h | 2 +- libs/ardour/midi_model.cc | 35 +++++++++---------- libs/ardour/midi_source.cc | 11 +++--- libs/evoral/evoral/Sequence.hpp | 32 ++++++++++++----- libs/evoral/src/Sequence.cpp | 75 ++++++++-------------------------------- 7 files changed, 78 insertions(+), 96 deletions(-) diff --git a/gtk2_ardour/midi_region_view.cc b/gtk2_ardour/midi_region_view.cc index ad6e74c0cf..b8f30a5da8 100644 --- a/gtk2_ardour/midi_region_view.cc +++ b/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 (); 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 @@ -165,6 +165,21 @@ public: boost::shared_ptr > find_note (boost::shared_ptr >); +private: + struct WriteLockImpl : public AutomatableSequence::WriteLockImpl { + WriteLockImpl(Glib::Mutex::Lock* source_lock, Glib::RWLock& s, Glib::Mutex& c) + : AutomatableSequence::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; 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::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 > 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 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 source) set_percussive(old_percussive); - read_unlock(); set_edited(false); return true; @@ -731,3 +722,11 @@ MidiModel::find_note (boost::shared_ptr > other) return boost::shared_ptr >(); } + +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& dst, sframes_t source_start, Evoral::Sequence::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; diff --git a/libs/evoral/evoral/Sequence.hpp b/libs/evoral/evoral/Sequence.hpp index b5737638e1..21db34eb68 100644 --- a/libs/evoral/evoral/Sequence.hpp +++ b/libs/evoral/evoral/Sequence.hpp @@ -63,11 +63,26 @@ class Sequence : virtual public ControlSet { public: Sequence(const TypeMap& type_map); - void write_lock(); - void write_unlock(); +protected: + 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; + }; + +public: + typedef boost::shared_ptr ReadLock; + typedef boost::shared_ptr WriteLock; - void read_lock() const; - void read_unlock() const; + 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