diff options
author | David Robillard <d@drobilla.net> | 2009-10-22 16:15:36 +0000 |
---|---|---|
committer | David Robillard <d@drobilla.net> | 2009-10-22 16:15:36 +0000 |
commit | d98c8e8fa4bb18cbbf2a18b82b11d9949bb00890 (patch) | |
tree | fd0a5fa975e491c1641417e8f121159678a3a8ce | |
parent | 2eed368c24f3411dc062a90058b1d6be5705444d (diff) |
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
-rw-r--r-- | gtk2_ardour/midi_region_view.cc | 4 | ||||
-rw-r--r-- | libs/ardour/ardour/midi_model.h | 15 | ||||
-rw-r--r-- | libs/ardour/ardour/midi_source.h | 2 | ||||
-rw-r--r-- | libs/ardour/midi_model.cc | 35 | ||||
-rw-r--r-- | libs/ardour/midi_source.cc | 11 | ||||
-rw-r--r-- | libs/evoral/evoral/Sequence.hpp | 32 | ||||
-rw-r--r-- | 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 @@ -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; 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<Glib::RWLock::ReaderLock> ReadLock; + typedef boost::shared_ptr<WriteLockImpl> 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<Time>& ev); - mutable Glib::RWLock _lock; - const TypeMap& _type_map; Notes _notes; diff --git a/libs/evoral/src/Sequence.cpp b/libs/evoral/src/Sequence.cpp index 4ba64fb12d..32191982c2 100644 --- a/libs/evoral/src/Sequence.cpp +++ b/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) { - _seq->read_unlock(); - _locked = false; - } + _lock.reset(); } 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> |