From f219a53744c3ccced52070a0ebab5fbe7f9b9895 Mon Sep 17 00:00:00 2001 From: David Robillard Date: Tue, 17 Feb 2009 06:09:37 +0000 Subject: [PATCH] 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 | 5 +++ libs/ardour/ardour/source.h | 5 ++- libs/ardour/midi_model.cc | 72 +++++++++++++------------------- libs/ardour/midi_source.cc | 8 +++- libs/evoral/evoral/Sequence.hpp | 10 ++--- libs/evoral/src/Sequence.cpp | 24 +++++++++-- 6 files changed, 68 insertions(+), 56 deletions(-) diff --git a/libs/ardour/ardour/midi_source.h b/libs/ardour/ardour/midi_source.h index 0bc431a8d8..b17f78b912 100644 --- 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 943b7b444c..d89c33c749 100644 --- 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& time_converter() const { return Evoral::IdentityConverter(); } - - 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 6573bcfd68..e7eaf64d32 100644 --- 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(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 m, - const std::string& name) +MidiModel::DeltaCommand::DeltaCommand(boost::shared_ptr m, const std::string& name) : Command(name) , _model(m) , _name(name) { } -MidiModel::DeltaCommand::DeltaCommand(boost::shared_ptr m, - const XMLNode& node) +MidiModel::DeltaCommand::DeltaCommand(boost::shared_ptr m, const XMLNode& node) : _model(m) { set_state(node); @@ -91,7 +89,6 @@ MidiModel::DeltaCommand::DeltaCommand(boost::shared_ptr m, void MidiModel::DeltaCommand::add(const boost::shared_ptr< Evoral::Note > 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 > n void MidiModel::DeltaCommand::remove(const boost::shared_ptr< Evoral::Note > 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 > 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 b539d5b9d3..06437852c6 100644 --- 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& 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& dst, nframes_t start, nframes_ Evoral::Sequence::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 30c1c1f0ca..ee18af50dc 100644 --- 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