VCA/SlavableAutomationCtrl re-design:
authorRobin Gareus <robin@gareus.org>
Mon, 12 Jun 2017 00:25:20 +0000 (02:25 +0200)
committerRobin Gareus <robin@gareus.org>
Mon, 12 Jun 2017 00:26:23 +0000 (02:26 +0200)
* remember master-ctrl value on assignment & save with session
* Control/AutomationCtrl only stores ctrl's own value (w/o master)
* virtual AutomationControl::get_value () -> use SlavableAC method
* MasterRecord uses weak-ptr (fixes recursive ~Controllable() deadlock)

libs/ardour/ardour/automation_control.h
libs/ardour/ardour/slavable_automation_control.h
libs/ardour/ardour/solo_control.h
libs/ardour/ardour/solo_isolate_control.h
libs/ardour/automation_control.cc
libs/ardour/slavable_automation_control.cc
libs/ardour/solo_control.cc
libs/ardour/solo_isolate_control.cc
libs/pbd/pbd/controllable.h

index a16929afa5333d02cee463fcc9be088ff1fdea64..98797678071f21956c9d6392d1966036265e9c44 100644 (file)
@@ -91,9 +91,10 @@ public:
        void start_touch(double when);
        void stop_touch(bool mark, double when);
 
-       /* inherited from PBD::Controllable.
-        */
-       double get_value () const;
+       /* inherited from PBD::Controllable. */
+       virtual double get_value () const;
+       virtual double get_save_value () const;
+
        /* inherited from PBD::Controllable.
         * Derived classes MUST call ::writable() to verify
         * that writing to the parameter is legal at that time.
index ceafc79193fb950ff4287aac75dbeeb8be31a4c5..29ab2e1055a63da2763601ebfe451b50b7ecddc7 100644 (file)
@@ -84,12 +84,21 @@ protected:
 
        class MasterRecord {
        public:
-               MasterRecord (boost::shared_ptr<AutomationControl> gc, double r)
+               MasterRecord (boost::weak_ptr<AutomationControl> gc, double vc, double vm)
                        : _master (gc)
                        , _yn (false)
+                       , _val_ctrl (vc)
+                       , _val_master (vm)
                {}
 
-               boost::shared_ptr<AutomationControl> master() const { return _master; }
+               boost::shared_ptr<AutomationControl> master() const { assert(_master.lock()); return _master.lock(); }
+
+               double val_ctrl () const { return _val_ctrl; }
+               double val_master () const { return _val_master; }
+
+               double master_ratio () const { return _val_master == 0 ? master()->get_value() : master()->get_value() / _val_master; }
+
+               int set_state (XMLNode const&, int);
 
                /* for boolean/toggled controls, we store a boolean value to
                 * indicate if this master returned true/false (1.0/0.0) from
@@ -99,18 +108,22 @@ protected:
                bool yn() const { return _yn; }
                void set_yn (bool yn) { _yn = yn; }
 
-               PBD::ScopedConnection connection;
+               PBD::ScopedConnection changed_connection;
+               PBD::ScopedConnection dropped_connection;
 
   private:
-               boost::shared_ptr<AutomationControl> _master;
+               boost::weak_ptr<AutomationControl> _master;
                /* holds most recently seen master value for boolean/toggle controls */
                bool   _yn;
+
+               /* values at time of assignment */
+               double _val_ctrl;
+               double _val_master;
        };
 
        mutable Glib::Threads::RWLock master_lock;
        typedef std::map<PBD::ID,MasterRecord> Masters;
        Masters _masters;
-       std::map<boost::weak_ptr<AutomationControl>, PBD::ScopedConnection> masters_connections;
 
        void   master_going_away (boost::weak_ptr<AutomationControl>);
        double get_value_locked() const;
@@ -121,7 +134,7 @@ protected:
        virtual bool boolean_automation_run_locked (framepos_t start, pframes_t len);
        bool boolean_automation_run (framepos_t start, pframes_t len);
 
-       virtual void   master_changed (bool from_self, GroupControlDisposition gcd, boost::shared_ptr<AutomationControl>);
+       virtual void   master_changed (bool from_self, GroupControlDisposition gcd, boost::weak_ptr<AutomationControl>);
        virtual double get_masters_value_locked () const;
        virtual void   pre_remove_master (boost::shared_ptr<AutomationControl>) {}
        virtual void   post_add_master (boost::shared_ptr<AutomationControl>) {}
index cc8698358262835600d0a036b951f57e446187c2..8026e5ea26760e1346038b2f466108fbc8e11855 100644 (file)
@@ -98,7 +98,7 @@ class LIBARDOUR_API SoloControl : public SlavableAutomationControl
 
   protected:
        void actually_set_value (double, PBD::Controllable::GroupControlDisposition group_override);
-       void master_changed (bool from_self, GroupControlDisposition, boost::shared_ptr<AutomationControl> m);
+       void master_changed (bool from_self, GroupControlDisposition, boost::weak_ptr<AutomationControl> m);
        void pre_remove_master (boost::shared_ptr<AutomationControl>);
        void post_add_master (boost::shared_ptr<AutomationControl>);
 
index 1e49469067333186907ae496a0a680ba9856a98a..190e0cdda3a01257d92e71c20f40f78309b43a8f 100644 (file)
@@ -73,7 +73,7 @@ class LIBARDOUR_API SoloIsolateControl : public SlavableAutomationControl
        XMLNode& get_state ();
 
   protected:
-       void master_changed (bool from_self, PBD::Controllable::GroupControlDisposition gcd, boost::shared_ptr<AutomationControl>);
+       void master_changed (bool from_self, PBD::Controllable::GroupControlDisposition gcd, boost::weak_ptr<AutomationControl>);
        void actually_set_value (double, PBD::Controllable::GroupControlDisposition group_override);
 
   private:
index 4e81de8aaffb067d917d07fb2b434aeb6d9c4f1b..055c000bc9b9fec97a0a74389fa48cd19c206bfd 100644 (file)
@@ -92,6 +92,13 @@ AutomationControl::get_value() const
        return Control::get_double (from_list, _session.transport_frame());
 }
 
+double
+AutomationControl::get_save_value() const
+{
+       /* save user-value, not incl masters */
+       return Control::get_double ();
+}
+
 void
 AutomationControl::pre_realtime_queue_stuff (double val, PBD::Controllable::GroupControlDisposition gcd)
 {
index 5ab968f69cee011b271d1495a2ef7a1857f4f746..adde7cdac34bc45653d00a27564f08d6e4878f0d 100644 (file)
@@ -57,7 +57,6 @@ SlavableAutomationControl::~SlavableAutomationControl ()
 double
 SlavableAutomationControl::get_masters_value_locked () const
 {
-
        if (_desc.toggled) {
                for (Masters::const_iterator mr = _masters.begin(); mr != _masters.end(); ++mr) {
                        if (mr->second.master()->get_value()) {
@@ -70,7 +69,7 @@ SlavableAutomationControl::get_masters_value_locked () const
                double v = 1.0; /* the masters function as a scaling factor */
 
                for (Masters::const_iterator mr = _masters.begin(); mr != _masters.end(); ++mr) {
-                       v *= mr->second.master()->get_value ();
+                       v *= mr->second.master_ratio ();
                }
 
                return v;
@@ -146,6 +145,7 @@ SlavableAutomationControl::masters_curve_multiply (framepos_t start, framepos_t
                        = boost::dynamic_pointer_cast<SlavableAutomationControl>(mr->second.master());
                assert (sc);
                rv |= sc->masters_curve_multiply (start, end, vec, veclen);
+               apply_gain_to_buffer (vec, veclen, mr->second.master_ratio ());
        }
        return rv;
 }
@@ -179,38 +179,19 @@ SlavableAutomationControl::add_master (boost::shared_ptr<AutomationControl> m, b
        std::pair<Masters::iterator,bool> res;
 
        {
+               const double master_value = m->get_value();
                Glib::Threads::RWLock::WriterLock lm (master_lock);
 
-               pair<PBD::ID,MasterRecord> newpair (m->id(), MasterRecord (m, 1.0));
+               pair<PBD::ID,MasterRecord> newpair (m->id(), MasterRecord (boost::weak_ptr<AutomationControl> (m), get_value_locked(), master_value));
                res = _masters.insert (newpair);
 
                if (res.second) {
 
-                       if (!loading) {
-
-                               if (!_desc.toggled) {
-                                       const double master_value = m->get_value();
-
-                                       if (master_value == 0.0) {
-                                               AutomationControl::set_double (0.0, Controllable::NoGroup);
-                                       } else {
-                                               /* scale control's own value by
-                                                  amount that the master will
-                                                  contribute.
-                                               */
-                                               AutomationControl::set_double ((Control::get_double() / master_value), Controllable::NoGroup);
-                                       }
-                               }
-                       }
-
                        /* note that we bind @param m as a weak_ptr<AutomationControl>, thus
                           avoiding holding a reference to the control in the binding
                           itself.
                        */
-                       assert (masters_connections.find (boost::weak_ptr<AutomationControl>(m)) == masters_connections.end());
-                       PBD::ScopedConnection con;
-                       m->DropReferences.connect_same_thread (con, boost::bind (&SlavableAutomationControl::master_going_away, this, boost::weak_ptr<AutomationControl>(m)));
-                       masters_connections[boost::weak_ptr<AutomationControl>(m)] = con;
+                       m->DropReferences.connect_same_thread (res.first->second.dropped_connection, boost::bind (&SlavableAutomationControl::master_going_away, this, boost::weak_ptr<AutomationControl>(m)));
 
                        /* Store the connection inside the MasterRecord, so
                           that when we destroy it, the connection is destroyed
@@ -228,7 +209,7 @@ SlavableAutomationControl::add_master (boost::shared_ptr<AutomationControl> m, b
                           because the change came from the master.
                        */
 
-                       m->Changed.connect_same_thread (res.first->second.connection, boost::bind (&SlavableAutomationControl::master_changed, this, _1, _2, m));
+                       m->Changed.connect_same_thread (res.first->second.changed_connection, boost::bind (&SlavableAutomationControl::master_changed, this, _1, _2, boost::weak_ptr<AutomationControl>(m)));
                }
        }
 
@@ -295,14 +276,18 @@ SlavableAutomationControl::update_boolean_masters_records (boost::shared_ptr<Aut
 }
 
 void
-SlavableAutomationControl::master_changed (bool /*from_self*/, GroupControlDisposition gcd, boost::shared_ptr<AutomationControl> m)
+SlavableAutomationControl::master_changed (bool /*from_self*/, GroupControlDisposition gcd, boost::weak_ptr<AutomationControl> wm)
 {
+       boost::shared_ptr<AutomationControl> m = wm.lock ();
+       assert (m);
        Glib::Threads::RWLock::ReaderLock lm (master_lock, Glib::Threads::TRY_LOCK);
        if (!lm.locked ()) {
                /* boolean_automation_run_locked () special case */
                return;
        }
        bool send_signal = handle_master_change (m);
+       lm.release (); // update_boolean_masters_records() takes lock
+
        update_boolean_masters_records (m);
        if (send_signal) {
                Changed (false, Controllable::NoGroup); /* EMIT SIGNAL */
@@ -321,38 +306,32 @@ SlavableAutomationControl::master_going_away (boost::weak_ptr<AutomationControl>
 void
 SlavableAutomationControl::remove_master (boost::shared_ptr<AutomationControl> m)
 {
+       if (_session.deletion_in_progress()) {
+               /* no reason to care about new values or sending signals */
+               return;
+       }
+
        pre_remove_master (m);
+       double new_val = AutomationControl::get_double();
+       const double old_val = new_val;
 
        {
                Glib::Threads::RWLock::WriterLock lm (master_lock);
 
-               masters_connections.erase (boost::weak_ptr<AutomationControl>(m));
-               if (!_masters.erase (m->id())) {
-                       return;
-               }
-
-               if (!_session.deletion_in_progress()) {
+               Masters::const_iterator mi = _masters.find (m->id ());
 
-                       const double master_value = m->get_value ();
+               /* when un-assigning we apply the master-value permanently */
+               if (mi != _masters.end()) {
+                       new_val *= mi->second.master_ratio ();
+               }
 
-                       if (master_value == 0.0) {
-                               /* slave would have been set to 0.0 as well,
-                                  so just leave it there, and the user can
-                                  bring it back up. this fits with the
-                                  "removing a VCA does not change the level" rule.
-                               */
-                       } else {
-                               /* bump up the control's own value by the level
-                                  of the master that is being removed.
-                               */
-                               AutomationControl::set_double (AutomationControl::get_double() * master_value, Controllable::NoGroup);
-                       }
+               if (!_masters.erase (m->id())) {
+                       return;
                }
        }
 
-       if (_session.deletion_in_progress()) {
-               /* no reason to care about new values or sending signals */
-               return;
+       if (old_val != new_val) {
+               AutomationControl::set_double (new_val, Controllable::NoGroup);
        }
 
        MasterStatusChange (); /* EMIT SIGNAL */
@@ -365,31 +344,32 @@ SlavableAutomationControl::remove_master (boost::shared_ptr<AutomationControl> m
 void
 SlavableAutomationControl::clear_masters ()
 {
-       double current_value;
-       double new_value;
-       bool had_masters = false;
+       if (_session.deletion_in_progress()) {
+               /* no reason to care about new values or sending signals */
+               return;
+       }
+
+       double new_val = AutomationControl::get_double();
+       const double old_val = new_val;
 
        /* null ptr means "all masters */
        pre_remove_master (boost::shared_ptr<AutomationControl>());
 
        {
                Glib::Threads::RWLock::WriterLock lm (master_lock);
-               current_value = get_value_locked ();
-               if (!_masters.empty()) {
-                       had_masters = true;
+               if (_masters.empty()) {
+                       return;
                }
-               _masters.clear ();
-               masters_connections.clear ();
-               new_value = get_value_locked ();
-       }
+               /* permanently apply masters value */
+               new_val *= get_masters_value_locked ();
 
-       if (had_masters) {
-               MasterStatusChange (); /* EMIT SIGNAL */
+               _masters.clear ();
        }
 
-       if (new_value != current_value) {
-               actually_set_value (current_value, Controllable::UseGroup);
+       if (old_val != new_val) {
+               AutomationControl::set_double (new_val, Controllable::NoGroup);
        }
+       MasterStatusChange (); /* EMIT SIGNAL */
 
        /* no need to update boolean masters records, since all MRs will have
         * been removed already.
@@ -446,6 +426,11 @@ SlavableAutomationControl::find_next_event_locked (double now, double end, Evora
 bool
 SlavableAutomationControl::handle_master_change (boost::shared_ptr<AutomationControl>)
 {
+       /* Derived classes can implement this for special cases (e.g. mute).
+        * This method is called with a ReaderLock (master_lock) held.
+        *
+        * return true if the changed master value resulted
+        * in a change of the control itself. */
        return true; // emit Changed
 }
 
@@ -517,6 +502,23 @@ SlavableAutomationControl::slaved () const
        return !_masters.empty();
 }
 
+int
+SlavableAutomationControl::MasterRecord::set_state (XMLNode const& n, int)
+{
+       bool yn;
+       double v;
+       if (n.get_property (X_("yn"), yn)) {
+               _yn = yn;
+       }
+       if (n.get_property (X_("val-ctrl"), v)) {
+               _val_ctrl = v;
+       }
+       if (n.get_property (X_("val-master"), v)) {
+               _val_master = v;
+       }
+       return 0;
+}
+
 void
 SlavableAutomationControl::use_saved_master_ratios ()
 {
@@ -526,28 +528,19 @@ SlavableAutomationControl::use_saved_master_ratios ()
 
        Glib::Threads::RWLock::ReaderLock lm (master_lock);
 
-       /* use stored state, do not recompute */
-
-       if (_desc.toggled) {
-
-               XMLNodeList nlist = _masters_node->children();
-               XMLNodeIterator niter;
-
-               for (niter = nlist.begin(); niter != nlist.end(); ++niter) {
-                       ID id_val;
-                       bool yn;
-                       if (!(*niter)->get_property (X_("id"), id_val) || !(*niter)->get_property (X_("yn"), yn)) {
-                               continue;
-                 }
+       XMLNodeList nlist = _masters_node->children();
+       XMLNodeIterator niter;
 
-                       Masters::iterator mi = _masters.find (id_val);
-                       if (mi != _masters.end()) {
-                               mi->second.set_yn (yn);
-                       }
+       for (niter = nlist.begin(); niter != nlist.end(); ++niter) {
+               ID id_val;
+               if (!(*niter)->get_property (X_("id"), id_val)) {
+                       continue;
                }
-
-       } else {
-
+               Masters::iterator mi = _masters.find (id_val);
+               if (mi == _masters.end()) {
+                       continue;
+               }
+               mi->second.set_state (**niter, Stateful::loading_state_version);
        }
 
        delete _masters_node;
@@ -566,23 +559,21 @@ SlavableAutomationControl::get_state ()
 
        {
                Glib::Threads::RWLock::ReaderLock lm (master_lock);
-
                if (!_masters.empty()) {
-
                        XMLNode* masters_node = new XMLNode (X_("masters"));
+                       for (Masters::iterator mr = _masters.begin(); mr != _masters.end(); ++mr) {
+                               XMLNode* mnode = new XMLNode (X_("master"));
+                               mnode->set_property (X_("id"), mr->second.master()->id());
 
-                       if (_desc.toggled) {
-                               for (Masters::iterator mr = _masters.begin(); mr != _masters.end(); ++mr) {
-                                       XMLNode* mnode = new XMLNode (X_("master"));
-                                       mnode->set_property (X_("id"), mr->second.master()->id());
+                               if (_desc.toggled) {
                                        mnode->set_property (X_("yn"), mr->second.yn());
-                                       masters_node->add_child_nocopy (*mnode);
+                               } else {
+                                       mnode->set_property (X_("val-ctrl"), mr->second.val_ctrl());
+                                       mnode->set_property (X_("val-master"), mr->second.val_master());
                                }
-                       } else {
-
+                               masters_node->add_child_nocopy (*mnode);
+                               node.add_child_nocopy (*masters_node);
                        }
-
-                       node.add_child_nocopy (*masters_node);
                }
        }
 
index 0b07f88c6b3ada4cc26320ec61733b6a868d66fa..9d898493dc0b1b8e198785437543023cb12389c6 100644 (file)
@@ -259,8 +259,10 @@ SoloControl::get_state ()
 }
 
 void
-SoloControl::master_changed (bool /*from self*/, GroupControlDisposition, boost::shared_ptr<AutomationControl> m)
+SoloControl::master_changed (bool /*from self*/, GroupControlDisposition, boost::weak_ptr<AutomationControl> wm)
 {
+       boost::shared_ptr<AutomationControl> m = wm.lock ();
+       assert (m);
        bool send_signal = false;
 
        _transition_into_solo = 0;
index 90132a951cb2a359a0e47e2a5ace1f4b0dbff84d..9be6e1a7d586e2513c043c3353e384dc51ccf411 100644 (file)
@@ -42,7 +42,7 @@ SoloIsolateControl::SoloIsolateControl (Session& session, std::string const & na
 }
 
 void
-SoloIsolateControl::master_changed (bool from_self, PBD::Controllable::GroupControlDisposition gcd, boost::shared_ptr<AutomationControl>)
+SoloIsolateControl::master_changed (bool from_self, PBD::Controllable::GroupControlDisposition gcd, boost::weak_ptr<AutomationControl>)
 {
        if (!_soloable.can_solo()) {
                return;
index 3b416b07fda7e6eb281041bfa37e9ca3218a7240..a255bd628459a83c77cb5db6277c2023eaefc46b 100644 (file)
@@ -138,7 +138,7 @@ class LIBPBD_API Controllable : public PBD::StatefulDestructible {
        PBD::Signal2<void,bool,PBD::Controllable::GroupControlDisposition> Changed;
 
        int set_state (const XMLNode&, int version);
-       XMLNode& get_state ();
+       virtual XMLNode& get_state ();
 
        std::string name()      const { return _name; }