From e644cb4577ad0e9d1d918ec75e7aae540dd358ff Mon Sep 17 00:00:00 2001 From: Robin Gareus Date: Thu, 7 Jan 2021 19:23:58 +0100 Subject: [PATCH] Do not hold RegionWriteLock while emitting signals Various playlist operations can change region-properties which results in Region::send_change being emitted while the Playlist::RegionWriteLock is held. This can result in recursive lock and/or deadlocks or crashes. e.g. Insert time -> Playlist::shift -> Region::RegionPropertyChanged -> EditorSummary::set_background_dirty -> Editor::session_gui_extents -> Playlist::get_extent -> read-lock is taken after write-lock. --- libs/ardour/ardour/midi_playlist.h | 2 +- libs/ardour/ardour/playlist.h | 40 +++++-- libs/ardour/midi_playlist.cc | 8 +- libs/ardour/playlist.cc | 165 ++++++++++++++--------------- 4 files changed, 114 insertions(+), 101 deletions(-) diff --git a/libs/ardour/ardour/midi_playlist.h b/libs/ardour/ardour/midi_playlist.h index 7e70707ae2..07de2cb705 100644 --- a/libs/ardour/ardour/midi_playlist.h +++ b/libs/ardour/ardour/midi_playlist.h @@ -77,7 +77,7 @@ public: int set_state (const XMLNode&, int version); bool destroy_region (boost::shared_ptr); - void _split_region (boost::shared_ptr, const MusicSample& position); + void _split_region (boost::shared_ptr, const MusicSample& position, ThawList& thawlist); void set_note_mode (NoteMode m) { _note_mode = m; } diff --git a/libs/ardour/ardour/playlist.h b/libs/ardour/ardour/playlist.h index bdf9788875..47b3438966 100644 --- a/libs/ardour/ardour/playlist.h +++ b/libs/ardour/ardour/playlist.h @@ -271,6 +271,26 @@ protected: friend class Session; protected: + class ThawList : public RegionList { + public: + void add (boost::shared_ptr r) + { + if (std::find (begin(), end(), r) != end ()) { + return; + } + r->suspend_property_changes (); + push_back (r); + } + + void release () + { + for (RegionList::iterator i = begin(); i != end(); ++i) { + (*i)->resume_property_changes (); + } + clear (); + } + }; + class RegionReadLock : public Glib::Threads::RWLock::ReaderLock { public: RegionReadLock (Playlist *pl) : Glib::Threads::RWLock::ReaderLock (pl->region_lock) {} @@ -293,7 +313,10 @@ protected: if (block_notify) { playlist->release_notifications (); } + thawlist.release (); } + + ThawList thawlist; Playlist *playlist; bool block_notify; }; @@ -376,27 +399,24 @@ protected: void sort_regions (); void possibly_splice (samplepos_t at, samplecnt_t distance, boost::shared_ptr exclude = boost::shared_ptr()); - void possibly_splice_unlocked(samplepos_t at, samplecnt_t distance, boost::shared_ptr exclude = boost::shared_ptr()); + void possibly_splice_unlocked (samplepos_t at, samplecnt_t distance, boost::shared_ptr exclude, ThawList& thawlist); - void core_splice (samplepos_t at, samplecnt_t distance, boost::shared_ptr exclude); void splice_locked (samplepos_t at, samplecnt_t distance, boost::shared_ptr exclude); - void splice_unlocked (samplepos_t at, samplecnt_t distance, boost::shared_ptr exclude); + void splice_unlocked (samplepos_t at, samplecnt_t distance, boost::shared_ptr exclude, ThawList& thawlist); - void core_ripple (samplepos_t at, samplecnt_t distance, RegionList *exclude); void ripple_locked (samplepos_t at, samplecnt_t distance, RegionList *exclude); - void ripple_unlocked (samplepos_t at, samplecnt_t distance, RegionList *exclude); - + void ripple_unlocked (samplepos_t at, samplecnt_t distance, RegionList *exclude, ThawList& thawlist); virtual void remove_dependents (boost::shared_ptr /*region*/) {} virtual void region_going_away (boost::weak_ptr /*region*/) {} virtual XMLNode& state (bool); - bool add_region_internal (boost::shared_ptr, samplepos_t position, int32_t sub_num = 0, double quarter_note = 0.0, bool for_music = false); + bool add_region_internal (boost::shared_ptr, samplepos_t position, ThawList& thawlist, int32_t sub_num = 0, double quarter_note = 0.0, bool for_music = false); - int remove_region_internal (boost::shared_ptr); + int remove_region_internal (boost::shared_ptr, ThawList& thawlist); void copy_regions (RegionList&) const; - void partition_internal (samplepos_t start, samplepos_t end, bool cutting, RegionList& thawlist); + void partition_internal (samplepos_t start, samplepos_t end, bool cutting, ThawList& thawlist); std::pair _get_extent() const; @@ -410,7 +430,7 @@ protected: void begin_undo (); void end_undo (); - virtual void _split_region (boost::shared_ptr, const MusicSample& position); + virtual void _split_region (boost::shared_ptr, const MusicSample& position, ThawList& thawlist); typedef std::pair, boost::shared_ptr > TwoRegions; diff --git a/libs/ardour/midi_playlist.cc b/libs/ardour/midi_playlist.cc index 4dc0d19e6f..1d385fc017 100644 --- a/libs/ardour/midi_playlist.cc +++ b/libs/ardour/midi_playlist.cc @@ -200,7 +200,7 @@ MidiPlaylist::destroy_region (boost::shared_ptr region) return changed; } void -MidiPlaylist::_split_region (boost::shared_ptr region, const MusicSample& playlist_position) +MidiPlaylist::_split_region (boost::shared_ptr region, const MusicSample& playlist_position, ThawList& thawlist) { if (!region->covers (playlist_position.sample)) { return; @@ -266,10 +266,10 @@ MidiPlaylist::_split_region (boost::shared_ptr region, const MusicSample right = RegionFactory::create (region, before, plist, true); } - add_region_internal (left, region->position(), 0, region->quarter_note(), true); - add_region_internal (right, region->position() + before.sample, before.division, region->quarter_note() + before_qn, true); + add_region_internal (left, region->position(), thawlist, 0, region->quarter_note(), true); + add_region_internal (right, region->position() + before.sample, thawlist, before.division, region->quarter_note() + before_qn, true); - remove_region_internal (region); + remove_region_internal (region, thawlist); _splicing = old_sp; } diff --git a/libs/ardour/playlist.cc b/libs/ardour/playlist.cc index 76676bd1cc..9d30782eb3 100644 --- a/libs/ardour/playlist.cc +++ b/libs/ardour/playlist.cc @@ -172,13 +172,15 @@ Playlist::Playlist (boost::shared_ptr other, string namestr, boo init (hide); RegionList tmp; + ThawList thawlist; other->copy_regions (tmp); in_set_state++; for (list >::iterator x = tmp.begin(); x != tmp.end(); ++x) { - add_region_internal( (*x), (*x)->position()); + add_region_internal ((*x), (*x)->position(), thawlist); } + thawlist.release (); in_set_state--; @@ -210,6 +212,7 @@ Playlist::Playlist (boost::shared_ptr other, samplepos_t start, in_set_state++; + ThawList thawlist; for (RegionList::const_iterator i = other->regions.begin(); i != other->regions.end(); ++i) { boost::shared_ptr region; @@ -280,9 +283,11 @@ Playlist::Playlist (boost::shared_ptr other, samplepos_t start, new_region = RegionFactory::create (region, plist); - add_region_internal (new_region, position); + add_region_internal (new_region, position, thawlist); } + thawlist.release (); + //keep track of any dead space at end (for pasting into Ripple or Splice mode) //at the end of construction, any length of cnt beyond the extents of the regions is end_space _end_space = cnt - (get_extent().second - get_extent().first); @@ -701,7 +706,7 @@ Playlist::add_region (boost::shared_ptr region, samplepos_t position, fl if (times == 1 && auto_partition){ RegionList thawlist; - partition_internal (pos - 1, (pos + region->length()), true, thawlist); + partition_internal (pos - 1, (pos + region->length()), true, rlock.thawlist); for (RegionList::iterator i = thawlist.begin(); i != thawlist.end(); ++i) { (*i)->resume_property_changes (); _session.add_command (new StatefulDiffCommand (*i)); @@ -709,7 +714,7 @@ Playlist::add_region (boost::shared_ptr region, samplepos_t position, fl } if (itimes >= 1) { - add_region_internal (region, pos, sub_num, quarter_note, for_music); + add_region_internal (region, pos, rlock.thawlist, sub_num, quarter_note, for_music); set_layer (region, DBL_MAX); pos += region->length(); --itimes; @@ -721,7 +726,7 @@ Playlist::add_region (boost::shared_ptr region, samplepos_t position, fl for (int i = 0; i < itimes; ++i) { boost::shared_ptr copy = RegionFactory::create (region, true); - add_region_internal (copy, pos, sub_num); + add_region_internal (copy, pos, rlock.thawlist, sub_num); set_layer (copy, DBL_MAX); pos += region->length(); } @@ -742,12 +747,12 @@ Playlist::add_region (boost::shared_ptr region, samplepos_t position, fl plist.add (Properties::layer, region->layer()); boost::shared_ptr sub = RegionFactory::create (region, plist); - add_region_internal (sub, pos, sub_num); + add_region_internal (sub, pos, rlock.thawlist, sub_num); set_layer (sub, DBL_MAX); } } - possibly_splice_unlocked (position, (pos + length) - position, region); + possibly_splice_unlocked (position, (pos + length) - position, region, rlock.thawlist); } void @@ -763,12 +768,17 @@ Playlist::set_region_ownership () } bool -Playlist::add_region_internal (boost::shared_ptr region, samplepos_t position, int32_t sub_num, double quarter_note, bool for_music) +Playlist::add_region_internal (boost::shared_ptr region, samplepos_t position, ThawList& thawlist, int32_t sub_num, double quarter_note, bool for_music) { if (region->data_type() != _type) { return false; } + /* note, this will delay signal emission and trigger Playlist::region_changed_proxy + * via PropertyChanged subsciption below :( + */ + thawlist.add (region); + RegionSortByPosition cmp; if (!first_set_state) { @@ -784,7 +794,7 @@ Playlist::add_region_internal (boost::shared_ptr region, samplepos_t pos regions.insert (upper_bound (regions.begin(), regions.end(), region, cmp), region); all_regions.insert (region); - possibly_splice_unlocked (position, region->length(), region); + possibly_splice_unlocked (position, region->length(), region, thawlist); if (!holding_state ()) { /* layers get assigned from XML state, and are not reset during undo/redo */ @@ -809,24 +819,24 @@ Playlist::replace_region (boost::shared_ptr old, boost::shared_ptrlayer ()); _splicing = old_sp; - possibly_splice_unlocked (pos, old->length() - newr->length()); + possibly_splice_unlocked (pos, old->length() - newr->length(), boost::shared_ptr(), rlock.thawlist); } void Playlist::remove_region (boost::shared_ptr region) { RegionWriteLock rlock (this); - remove_region_internal (region); + remove_region_internal (region, rlock.thawlist); } int -Playlist::remove_region_internal (boost::shared_ptr region) +Playlist::remove_region_internal (boost::shared_ptr region, ThawList& thawlist) { RegionList::iterator i; @@ -845,7 +855,7 @@ Playlist::remove_region_internal (boost::shared_ptr region) regions.erase (i); - possibly_splice_unlocked (pos, -distance); + possibly_splice_unlocked (pos, -distance, boost::shared_ptr(), thawlist); if (!holding_state ()) { relayer (); @@ -920,15 +930,8 @@ Playlist::get_source_equivalent_regions (boost::shared_ptr other, vector void Playlist::partition (samplepos_t start, samplepos_t end, bool cut) { - RegionList thawlist; - { - RegionWriteLock lock(this); - partition_internal (start, end, cut, thawlist); - } - - for (RegionList::iterator i = thawlist.begin(); i != thawlist.end(); ++i) { - (*i)->resume_property_changes (); - } + RegionWriteLock lock(this); + partition_internal (start, end, cut, lock.thawlist); } /* If a MIDI region is locked to musical-time, Properties::start is ignored @@ -950,7 +953,7 @@ static void maybe_add_start_beats (TempoMap const& tm, PropertyList& plist, boos * removed. */ void -Playlist::partition_internal (samplepos_t start, samplepos_t end, bool cutting, RegionList& thawlist) +Playlist::partition_internal (samplepos_t start, samplepos_t end, bool cutting, ThawList& thawlist) { RegionList new_regions; @@ -981,7 +984,7 @@ Playlist::partition_internal (samplepos_t start, samplepos_t end, bool cutting, if (current->first_sample() >= start && current->last_sample() < end) { if (cutting) { - remove_region_internal (current); + remove_region_internal (current, thawlist); } continue; @@ -1040,7 +1043,7 @@ Playlist::partition_internal (samplepos_t start, samplepos_t end, bool cutting, * for MusicSample is needed to offset region-gain */ region = RegionFactory::create (current, MusicSample (pos2 - pos1, 0), plist); - add_region_internal (region, start); + add_region_internal (region, start, thawlist); new_regions.push_back (region); } @@ -1061,14 +1064,13 @@ Playlist::partition_internal (samplepos_t start, samplepos_t end, bool cutting, region = RegionFactory::create (current, MusicSample (pos3 - pos1, 0), plist); - add_region_internal (region, end); + add_region_internal (region, end, thawlist); new_regions.push_back (region); /* "front" ***** */ current->clear_changes (); - current->suspend_property_changes (); - thawlist.push_back (current); + thawlist.add (current); current->cut_end (pos2 - 1); } else if (overlap == Evoral::OverlapEnd) { @@ -1102,15 +1104,14 @@ Playlist::partition_internal (samplepos_t start, samplepos_t end, bool cutting, region = RegionFactory::create (current, MusicSample(pos2 - pos1, 0), plist); - add_region_internal (region, start); + add_region_internal (region, start, thawlist); new_regions.push_back (region); } /* front ****** */ current->clear_changes (); - current->suspend_property_changes (); - thawlist.push_back (current); + thawlist.add (current); current->cut_end (pos2 - 1); } else if (overlap == Evoral::OverlapStart) { @@ -1146,15 +1147,14 @@ Playlist::partition_internal (samplepos_t start, samplepos_t end, bool cutting, region = RegionFactory::create (current, plist); - add_region_internal (region, pos1); + add_region_internal (region, pos1, thawlist); new_regions.push_back (region); } /* end */ current->clear_changes (); - current->suspend_property_changes (); - thawlist.push_back (current); + thawlist.add (current); current->trim_front (pos3); } else if (overlap == Evoral::OverlapExternal) { @@ -1175,7 +1175,7 @@ Playlist::partition_internal (samplepos_t start, samplepos_t end, bool cutting, */ if (cutting) { - remove_region_internal (current); + remove_region_internal (current, thawlist); } new_regions.push_back (current); @@ -1241,7 +1241,6 @@ boost::shared_ptr Playlist::cut (samplepos_t start, samplecnt_t cnt, bool result_is_hidden) { boost::shared_ptr the_copy; - RegionList thawlist; char buf[32]; snprintf (buf, sizeof (buf), "%" PRIu32, ++subcnt); @@ -1255,11 +1254,7 @@ Playlist::cut (samplepos_t start, samplecnt_t cnt, bool result_is_hidden) { RegionWriteLock rlock (this); - partition_internal (start, start+cnt-1, true, thawlist); - } - - for (RegionList::iterator i = thawlist.begin(); i != thawlist.end(); ++i) { - (*i)->resume_property_changes(); + partition_internal (start, start+cnt-1, true, rlock.thawlist); } return the_copy; @@ -1303,7 +1298,7 @@ Playlist::paste (boost::shared_ptr other, samplepos_t position, float the ordering they had in the original playlist. */ - add_region_internal (copy_of_region, (*i)->position() + pos, sub_num); + add_region_internal (copy_of_region, (*i)->position() + pos, rl1.thawlist, sub_num); set_layer (copy_of_region, copy_of_region->layer() + top); } pos += shift; @@ -1332,7 +1327,7 @@ Playlist::duplicate (boost::shared_ptr region, samplepos_t position, sam while (itimes--) { boost::shared_ptr copy = RegionFactory::create (region, true); - add_region_internal (copy, position); + add_region_internal (copy, position, rl.thawlist); set_layer (copy, DBL_MAX); position += gap; } @@ -1350,7 +1345,7 @@ Playlist::duplicate (boost::shared_ptr region, samplepos_t position, sam plist.add (Properties::name, name); boost::shared_ptr sub = RegionFactory::create (region, plist); - add_region_internal (sub, position); + add_region_internal (sub, position, rl.thawlist); set_layer (sub, DBL_MAX); } } @@ -1365,7 +1360,7 @@ Playlist::duplicate_until (boost::shared_ptr region, samplepos_t positio while (position + region->length() - 1 < end) { boost::shared_ptr copy = RegionFactory::create (region, true); - add_region_internal (copy, position); + add_region_internal (copy, position, rl.thawlist); set_layer (copy, DBL_MAX); position += gap; } @@ -1383,7 +1378,7 @@ Playlist::duplicate_until (boost::shared_ptr region, samplepos_t positio plist.add (Properties::name, name); boost::shared_ptr sub = RegionFactory::create (region, plist); - add_region_internal (sub, position); + add_region_internal (sub, position, rl.thawlist); set_layer (sub, DBL_MAX); } } @@ -1458,6 +1453,7 @@ Playlist::shift (samplepos_t at, sampleoffset_t distance, bool move_intersected, continue; } + rlock.thawlist.add (*r); (*r)->set_position ((*r)->position() + distance); } @@ -1476,7 +1472,7 @@ Playlist::split (const MusicSample& at) /* use a copy since this operation can modify the region list */ for (RegionList::iterator r = copy.begin(); r != copy.end(); ++r) { - _split_region (*r, at); + _split_region (*r, at, rlock.thawlist); } } @@ -1484,11 +1480,11 @@ void Playlist::split_region (boost::shared_ptr region, const MusicSample& playlist_position) { RegionWriteLock rl (this); - _split_region (region, playlist_position); + _split_region (region, playlist_position, rl.thawlist); } void -Playlist::_split_region (boost::shared_ptr region, const MusicSample& playlist_position) +Playlist::_split_region (boost::shared_ptr region, const MusicSample& playlist_position, ThawList& thawlist) { if (!region->covers (playlist_position.sample)) { return; @@ -1545,10 +1541,10 @@ Playlist::_split_region (boost::shared_ptr region, const MusicSample& pl right = RegionFactory::create (region, before, plist, true); } - add_region_internal (left, region->position(), 0); - add_region_internal (right, region->position() + before.sample, before.division); + add_region_internal (left, region->position(), thawlist, 0); + add_region_internal (right, region->position() + before.sample, thawlist, before.division); - remove_region_internal (region); + remove_region_internal (region, thawlist); _splicing = old_sp; } @@ -1596,7 +1592,7 @@ Playlist::possibly_splice (samplepos_t at, samplecnt_t distance, boost::shared_p } void -Playlist::possibly_splice_unlocked (samplepos_t at, samplecnt_t distance, boost::shared_ptr exclude) +Playlist::possibly_splice_unlocked (samplepos_t at, samplecnt_t distance, boost::shared_ptr exclude, ThawList& thawlist) { if (_splicing || in_set_state) { /* don't respond to splicing moves or state setting */ @@ -1604,27 +1600,19 @@ Playlist::possibly_splice_unlocked (samplepos_t at, samplecnt_t distance, boost: } if (_edit_mode == Splice) { - splice_unlocked (at, distance, exclude); + splice_unlocked (at, distance, exclude, thawlist); } } void Playlist::splice_locked (samplepos_t at, samplecnt_t distance, boost::shared_ptr exclude) { - { - RegionWriteLock rl (this); - core_splice (at, distance, exclude); - } + RegionWriteLock rl (this); + splice_unlocked (at, distance, exclude, rl.thawlist); } void -Playlist::splice_unlocked (samplepos_t at, samplecnt_t distance, boost::shared_ptr exclude) -{ - core_splice (at, distance, exclude); -} - -void -Playlist::core_splice (samplepos_t at, samplecnt_t distance, boost::shared_ptr exclude) +Playlist::splice_unlocked (samplepos_t at, samplecnt_t distance, boost::shared_ptr exclude, ThawList& thawlist) { _splicing = true; @@ -1642,6 +1630,7 @@ Playlist::core_splice (samplepos_t at, samplecnt_t distance, boost::shared_ptrlength(); } + thawlist.add (*i); (*i)->set_position (new_pos); } } @@ -1654,20 +1643,12 @@ Playlist::core_splice (samplepos_t at, samplecnt_t distance, boost::shared_ptrset_position (new_pos); } } @@ -2266,13 +2248,16 @@ Playlist::update (const RegionListProperty::ChangeRecord& change) name(), change.added.size(), change.removed.size())); freeze (); - /* add the added regions */ - for (RegionListProperty::ChangeContainer::const_iterator i = change.added.begin(); i != change.added.end(); ++i) { - add_region_internal ((*i), (*i)->position()); - } - /* remove the removed regions */ - for (RegionListProperty::ChangeContainer::const_iterator i = change.removed.begin(); i != change.removed.end(); ++i) { - remove_region (*i); + { + RegionWriteLock rlock (this); + /* add the added regions */ + for (RegionListProperty::ChangeContainer::const_iterator i = change.added.begin(); i != change.added.end(); ++i) { + add_region_internal ((*i), (*i)->position(), rlock.thawlist); + } + /* remove the removed regions */ + for (RegionListProperty::ChangeContainer::const_iterator i = change.removed.begin(); i != change.removed.end(); ++i) { + remove_region_internal (*i, rlock.thawlist); + } } thaw (); @@ -2364,7 +2349,7 @@ Playlist::set_state (const XMLNode& node, int version) { RegionWriteLock rlock (this); - add_region_internal (region, region->position()); + add_region_internal (region, region->position(), rlock.thawlist); } region->resume_property_changes (); @@ -2778,6 +2763,7 @@ Playlist::nudge_after (samplepos_t start, samplecnt_t distance, bool forwards) } } + rlock.thawlist.add (*i); (*i)->set_position (new_pos); moved = true; } @@ -2952,6 +2938,9 @@ Playlist::shuffle (boost::shared_ptr region, int dir) new_pos = region->position() + (*next)->length(); } + rlock.thawlist.add (*next); + rlock.thawlist.add (region); + (*next)->set_position (region->position()); region->set_position (new_pos); @@ -2993,6 +2982,9 @@ Playlist::shuffle (boost::shared_ptr region, int dir) new_pos = (*prev)->position() + region->length(); } + rlock.thawlist.add (region); + rlock.thawlist.add (*prev); + region->set_position ((*prev)->position()); (*prev)->set_position (new_pos); @@ -3047,6 +3039,7 @@ Playlist::update_after_tempo_map_change () freeze (); for (RegionList::iterator i = copy.begin(); i != copy.end(); ++i) { + rlock.thawlist.add (*i); (*i)->update_after_tempo_map_change (); } /* possibly causes a contents changed notification (flush_notifications()) */