Fix deadlock and potential race condition when editing MIDI.

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

diff --git a/libs/ardour/ardour/midi_source.h b/libs/ardour/ardour/midi_source.h
index 0bc431a8d8ba0918cbb7a00a08a323a132cfda00..b17f78b9125e96ac1fc07140bedc13f7a08858bb 100644 (file)
--- a/libs/ardour/ardour/midi_source.h
+++ b/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);

diff --git a/libs/ardour/ardour/source.h b/libs/ardour/ardour/source.h
index 943b7b444cca6e9d94acec3462a6be1a99465c95..d89c33c74975e6cae9e2eb7dd482b853fec043ed 100644 (file)
--- a/libs/ardour/ardour/source.h
+++ b/libs/ardour/ardour/source.h
@@ -115,8 +115,9 @@ class Source : public SessionObject, public ARDOUR::Readable, public boost::nonc
        virtual const Evoral::TimeConverter<double, nframes_t>& time_converter() const {
                return Evoral::IdentityConverter<double, nframes_t>();
        }
-       
-       Flag flags() const { return _flags; }
+
+       Glib::Mutex& mutex()       { return _lock; }
+       Flag         flags() const { return _flags; }
 
   protected:
        DataType            _type;

diff --git a/libs/ardour/midi_model.cc b/libs/ardour/midi_model.cc
index 6573bcfd68bdbac4317a11f77d1d4034e2db7b2d..e7eaf64d32eab3524ef87797c67892bea3af4c90 100644 (file)
--- a/libs/ardour/midi_model.cc
+++ b/libs/ardour/midi_model.cc
@@ -1,22 +1,22 @@
 /*
- Copyright (C) 2007 Paul Davis 
- Written by Dave Robillard, 2007
+    Copyright (C) 2007 Paul Davis
+    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
- the Free Software Foundation; either version 2 of the License, or
- (at your option) any later version.
+    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
+    the Free Software Foundation; either version 2 of the License, or
+    (at your option) any later version.
 
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- GNU General Public License for more details.
+    This program is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
 
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+    You should have received a copy of the GNU General Public License
+    along with this program; if not, write to the Free Software
+    Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
 
- */
+*/
 
 #define __STDC_LIMIT_MACROS 1
 
@@ -37,11 +37,10 @@ using namespace std;
 using namespace ARDOUR;
 using namespace PBD;
 
-MidiModel::MidiModel(MidiSource *s, size_t size)
+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,16 +137,13 @@ 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 */
 }
 
 XMLNode&
 MidiModel::DeltaCommand::marshal_note(const boost::shared_ptr< Evoral::Note<TimeType> > note)
 {
-       XMLNode *xml_note = new XMLNode("note");
+       XMLNode* xml_note = new XMLNode("note");
        ostringstream note_str(ios::ate);
        note_str << int(note->note());
        xml_note->add_property("note", note_str.str());
@@ -248,13 +234,13 @@ MidiModel::DeltaCommand::set_state(const XMLNode& delta_command)
        }
 
        _added_notes.clear();
-       XMLNode *added_notes = delta_command.child(ADDED_NOTES_ELEMENT);
+       XMLNode* added_notes = delta_command.child(ADDED_NOTES_ELEMENT);
        XMLNodeList notes = added_notes->children();
        transform(notes.begin(), notes.end(), back_inserter(_added_notes),
                        sigc::mem_fun(*this, &DeltaCommand::unmarshal_note));
 
        _removed_notes.clear();
-       XMLNode *removed_notes = delta_command.child(REMOVED_NOTES_ELEMENT);
+       XMLNode* removed_notes = delta_command.child(REMOVED_NOTES_ELEMENT);
        notes = removed_notes->children();
        transform(notes.begin(), notes.end(), back_inserter(_removed_notes),
                        sigc::mem_fun(*this, &DeltaCommand::unmarshal_note));
@@ -265,15 +251,15 @@ MidiModel::DeltaCommand::set_state(const XMLNode& delta_command)
 XMLNode&
 MidiModel::DeltaCommand::get_state()
 {
-       XMLNode *delta_command = new XMLNode(DELTA_COMMAND_ELEMENT);
+       XMLNode* delta_command = new XMLNode(DELTA_COMMAND_ELEMENT);
        delta_command->add_property("midi-source", _model->midi_source()->id().to_s());
 
-       XMLNode *added_notes = delta_command->add_child(ADDED_NOTES_ELEMENT);
+       XMLNode* added_notes = delta_command->add_child(ADDED_NOTES_ELEMENT);
        for_each(_added_notes.begin(), _added_notes.end(), sigc::compose(
                        sigc::mem_fun(*added_notes, &XMLNode::add_child_nocopy),
                        sigc::mem_fun(*this, &DeltaCommand::marshal_note)));
 
-       XMLNode *removed_notes = delta_command->add_child(REMOVED_NOTES_ELEMENT);
+       XMLNode* removed_notes = delta_command->add_child(REMOVED_NOTES_ELEMENT);
        for_each(_removed_notes.begin(), _removed_notes.end(), sigc::compose(
                        sigc::mem_fun(*removed_notes, &XMLNode::add_child_nocopy),
                        sigc::mem_fun(*this, &DeltaCommand::marshal_note)));

diff --git a/libs/ardour/midi_source.cc b/libs/ardour/midi_source.cc
index b539d5b9d392e6d0bf3b1bd7c9b37d5a3b61279b..06437852c61a0f740fef37ad131783bd617e0427 100644 (file)
--- a/libs/ardour/midi_source.cc
+++ b/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) {

diff --git a/libs/evoral/evoral/Sequence.hpp b/libs/evoral/evoral/Sequence.hpp
index 30c1c1f0ca8e9360cf794ea3a4ca6401a393d6bc..ee18af50dc4cb31db10ab091f5282f89c3f02804 100644 (file)
--- a/libs/evoral/evoral/Sequence.hpp
+++ b/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();
 
@@ -126,6 +124,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; }
@@ -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,8 +172,7 @@ public:
        uint8_t highest_note() const { return _highest_note; }
        
 protected:
-       mutable const_iterator _read_iter;
-       bool                   _edited;
+       bool _edited;
 
 private:
        friend class const_iterator;

diff --git a/libs/evoral/src/Sequence.cpp b/libs/evoral/src/Sequence.cpp
index c5deda554bf6f60acfc291e67e6b6ea2a9de3e60..4328332c5b901b2b858b5547efb6a41a68a18abb 100644 (file)
--- a/libs/evoral/src/Sequence.cpp
+++ b/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();
 }