From f95c9216b147d4ac153fe2792897962fd6cbacf0 Mon Sep 17 00:00:00 2001 From: Robin Gareus Date: Sat, 10 Jun 2023 03:09:41 +0200 Subject: [PATCH] Fix playlist use-count 2/2 * Use an atomic reference count since the freeze-thread can call use_playlist. * Remove explicit argument to construct unused playlist because playlists are unused by default. This also lead to use-count becoming negative (or rather UINT32_MAX) --- libs/ardour/ardour/playlist.h | 4 ++-- libs/ardour/ardour/playlist_factory.h | 4 ++-- libs/ardour/ardour/session.h | 2 +- libs/ardour/audio_playlist_importer.cc | 2 +- libs/ardour/audio_track.cc | 2 +- libs/ardour/midi_track.cc | 3 +++ libs/ardour/playlist.cc | 21 ++++++++++++++------- libs/ardour/playlist_factory.cc | 10 +++++----- libs/ardour/playlist_source.cc | 8 +++++++- libs/ardour/session.cc | 6 +----- libs/ardour/session_playlists.cc | 6 +++++- libs/ardour/session_state.cc | 2 +- libs/ardour/source_factory.cc | 3 ++- 13 files changed, 45 insertions(+), 28 deletions(-) diff --git a/libs/ardour/ardour/playlist.h b/libs/ardour/ardour/playlist.h index 20cb171552..d01910a53e 100644 --- a/libs/ardour/ardour/playlist.h +++ b/libs/ardour/ardour/playlist.h @@ -143,7 +143,7 @@ public: void release (); bool empty () const; - bool used () const { return _refcnt != 0; } + bool used () const { return _refcnt.load () != 0; } int sort_id () const { return _sort_id; } bool frozen () const { return _frozen; } const DataType& data_type () const { return _type; } @@ -377,7 +377,7 @@ protected: bool _rippling; bool _shuffling; bool _nudging; - uint32_t _refcnt; + std::atomic _refcnt; bool in_flush; bool in_partition; bool _frozen; diff --git a/libs/ardour/ardour/playlist_factory.h b/libs/ardour/ardour/playlist_factory.h index c369fc9581..7e641dd418 100644 --- a/libs/ardour/ardour/playlist_factory.h +++ b/libs/ardour/ardour/playlist_factory.h @@ -32,9 +32,9 @@ class Session; class LIBARDOUR_API PlaylistFactory { public: - static PBD::Signal2, bool> PlaylistCreated; + static PBD::Signal1> PlaylistCreated; - static std::shared_ptr create (Session&, const XMLNode&, bool hidden = false, bool unused = false); + static std::shared_ptr create (Session&, const XMLNode&, bool hidden = false); static std::shared_ptr create (DataType type, Session&, std::string name, bool hidden = false); static std::shared_ptr create (std::shared_ptr, std::string name, bool hidden = false); static std::shared_ptr create (std::shared_ptr, timepos_t const & start, timepos_t const & cnt, std::string name, bool hidden = false); diff --git a/libs/ardour/ardour/session.h b/libs/ardour/ardour/session.h index 8002b9f7cc..e0a3a0f0d8 100644 --- a/libs/ardour/ardour/session.h +++ b/libs/ardour/ardour/session.h @@ -902,7 +902,7 @@ public: std::shared_ptr midi_source_by_path (const std::string&, bool need_source_lock) const; uint32_t count_sources_by_origin (const std::string&); - void add_playlist (std::shared_ptr, bool unused = false); + void add_playlist (std::shared_ptr); /* Curves and AutomationLists (TODO when they go away) */ void add_automation_list(AutomationList*); diff --git a/libs/ardour/audio_playlist_importer.cc b/libs/ardour/audio_playlist_importer.cc index 597f861f9d..b1433e62ac 100644 --- a/libs/ardour/audio_playlist_importer.cc +++ b/libs/ardour/audio_playlist_importer.cc @@ -244,7 +244,7 @@ AudioPlaylistImporter::_move () } // Create playlist - playlist = PlaylistFactory::create (session, xml_playlist, false, true); + playlist = PlaylistFactory::create (session, xml_playlist, false); } void diff --git a/libs/ardour/audio_track.cc b/libs/ardour/audio_track.cc index 14d775cbe8..3c71f2fe0b 100644 --- a/libs/ardour/audio_track.cc +++ b/libs/ardour/audio_track.cc @@ -449,8 +449,8 @@ void AudioTrack::unfreeze () { if (_freeze_record.playlist) { - _freeze_record.playlist->release(); use_playlist (DataType::AUDIO, _freeze_record.playlist); + _freeze_record.playlist->release(); { Glib::Threads::RWLock::ReaderLock lm (_processor_lock); // should this be a write lock? jlc diff --git a/libs/ardour/midi_track.cc b/libs/ardour/midi_track.cc index 7acb91a105..d8305695df 100644 --- a/libs/ardour/midi_track.cc +++ b/libs/ardour/midi_track.cc @@ -96,6 +96,9 @@ MidiTrack::MidiTrack (Session& sess, string name, TrackMode mode) MidiTrack::~MidiTrack () { + if (_freeze_record.playlist && !_session.deletion_in_progress()) { + _freeze_record.playlist->release(); + } } int diff --git a/libs/ardour/playlist.cc b/libs/ardour/playlist.cc index e6bdfefdfd..07d194bdc6 100644 --- a/libs/ardour/playlist.cc +++ b/libs/ardour/playlist.cc @@ -287,18 +287,23 @@ Playlist::Playlist (std::shared_ptr other, timepos_t const & sta void Playlist::use () { - ++_refcnt; - InUse (true); /* EMIT SIGNAL */ + if (0 == _refcnt.fetch_add (1)) { + InUse (true); /* EMIT SIGNAL */ + } } void Playlist::release () { - if (_refcnt > 0) { - _refcnt--; + int oldval = _refcnt.fetch_sub (1); +#ifndef NDEBUG + if (oldval <= 0) { + cerr << "Bad Playlist::release for " << name() << endl; } + assert (oldval > 0); +#endif - if (_refcnt == 0) { + if (oldval == 1) { InUse (false); /* EMIT SIGNAL */ } } @@ -323,7 +328,6 @@ Playlist::init (bool hide) pending_contents_change = false; pending_layering = false; first_set_state = true; - _refcnt = 0; _hidden = hide; _rippling = false; _shuffling = false; @@ -335,7 +339,10 @@ Playlist::init (bool hide) subcnt = 0; _frozen = false; _capture_insertion_underway = false; - _combine_ops = 0; + _combine_ops = 0; + + _refcnt.store (0); + _end_space = timecnt_t (_type == DataType::AUDIO ? Temporal::AudioTime : Temporal::BeatTime); _playlist_shift_active = false; diff --git a/libs/ardour/playlist_factory.cc b/libs/ardour/playlist_factory.cc index d1a018bdba..e7849e33d7 100644 --- a/libs/ardour/playlist_factory.cc +++ b/libs/ardour/playlist_factory.cc @@ -32,10 +32,10 @@ using namespace std; using namespace ARDOUR; using namespace PBD; -PBD::Signal2, bool> PlaylistFactory::PlaylistCreated; +PBD::Signal1> PlaylistFactory::PlaylistCreated; std::shared_ptr -PlaylistFactory::create (Session& s, const XMLNode& node, bool hidden, bool unused) +PlaylistFactory::create (Session& s, const XMLNode& node, bool hidden) { XMLProperty const * type = node.property("type"); @@ -51,7 +51,7 @@ PlaylistFactory::create (Session& s, const XMLNode& node, bool hidden, bool unus pl->set_region_ownership (); if (pl && !hidden) { - PlaylistCreated (pl, unused); + PlaylistCreated (pl); } return pl; @@ -72,7 +72,7 @@ PlaylistFactory::create (DataType type, Session& s, string name, bool hidden) pl = std::shared_ptr (new MidiPlaylist (s, name, hidden)); if (pl && !hidden) { - PlaylistCreated (pl, false); + PlaylistCreated (pl); } return pl; @@ -99,7 +99,7 @@ PlaylistFactory::create (std::shared_ptr old, string name, bool } if (pl && !hidden) { - PlaylistCreated (pl, false); + PlaylistCreated (pl); } return pl; diff --git a/libs/ardour/playlist_source.cc b/libs/ardour/playlist_source.cc index b1ccd7f1d5..4fc6098788 100644 --- a/libs/ardour/playlist_source.cc +++ b/libs/ardour/playlist_source.cc @@ -119,9 +119,13 @@ PlaylistSource::set_state (const XMLNode& node, int /*version*/) nlist = node.children(); + if (_playlist) { + _playlist->release (); + } + for (niter = nlist.begin(); niter != nlist.end(); ++niter) { if ((*niter)->name() == "Playlist") { - _playlist = PlaylistFactory::create (_session, **niter, true, false); + _playlist = PlaylistFactory::create (_session, **niter, true); break; } } @@ -129,6 +133,8 @@ PlaylistSource::set_state (const XMLNode& node, int /*version*/) if (!_playlist) { error << _("Could not construct playlist for PlaylistSource from session data!") << endmsg; throw failed_constructor (); + } else { + _playlist->use (); } /* other properties */ diff --git a/libs/ardour/session.cc b/libs/ardour/session.cc index 47fc5e0d69..ad83420fcd 100644 --- a/libs/ardour/session.cc +++ b/libs/ardour/session.cc @@ -5151,7 +5151,7 @@ Session::playlist_is_active (std::shared_ptr playlist) } void -Session::add_playlist (std::shared_ptr playlist, bool unused) +Session::add_playlist (std::shared_ptr playlist) { if (playlist->hidden()) { return; @@ -5159,10 +5159,6 @@ Session::add_playlist (std::shared_ptr playlist, bool unused) _playlists->add (playlist); - if (unused) { - playlist->release(); - } - set_dirty(); } diff --git a/libs/ardour/session_playlists.cc b/libs/ardour/session_playlists.cc index a3b48d17aa..a1cd2427f8 100644 --- a/libs/ardour/session_playlists.cc +++ b/libs/ardour/session_playlists.cc @@ -78,7 +78,11 @@ SessionPlaylists::add (std::shared_ptr playlist) bool const existing = find (playlists.begin(), playlists.end(), playlist) != playlists.end(); if (!existing) { - playlists.insert (playlists.begin(), playlist); + if (playlist->used ()) { + playlists.insert (playlists.begin(), playlist); + } else { + unused_playlists.insert (unused_playlists.begin(), playlist); + } playlist->InUse.connect_same_thread (*this, boost::bind (&SessionPlaylists::track, this, _1, std::weak_ptr(playlist))); playlist->DropReferences.connect_same_thread ( *this, boost::bind (&SessionPlaylists::remove_weak, this, std::weak_ptr (playlist)) diff --git a/libs/ardour/session_state.cc b/libs/ardour/session_state.cc index 974b0b203a..c404731b85 100644 --- a/libs/ardour/session_state.cc +++ b/libs/ardour/session_state.cc @@ -210,7 +210,7 @@ Session::pre_engine_init (string fullpath) /* These are all static "per-class" signals */ SourceFactory::SourceCreated.connect_same_thread (*this, boost::bind (&Session::add_source, this, _1)); - PlaylistFactory::PlaylistCreated.connect_same_thread (*this, boost::bind (&Session::add_playlist, this, _1, _2)); + PlaylistFactory::PlaylistCreated.connect_same_thread (*this, boost::bind (&Session::add_playlist, this, _1)); AutomationList::AutomationListCreated.connect_same_thread (*this, boost::bind (&Session::add_automation_list, this, _1)); IO::PortCountChanged.connect_same_thread (*this, boost::bind (&Session::ensure_buffers, this, _1)); diff --git a/libs/ardour/source_factory.cc b/libs/ardour/source_factory.cc index 579965a4b1..527a09e5b4 100644 --- a/libs/ardour/source_factory.cc +++ b/libs/ardour/source_factory.cc @@ -417,7 +417,8 @@ SourceFactory::createFromPlaylist (DataType type, Session& s, std::shared_ptr ret (src); if (setup_peakfile (ret, defer_peaks)) {