various transient-detection fixes (split region, trim, move, undo,..)

* all API calls use session-time (allow region-lists)
* per-region transients are separated in
  - Onset (Rhythm Rodent, Aubio)
  - User-added
  - internal/source (QM), used as fallback for next/prev (read-only)

6 files changed, 245 insertions, 128 deletions

diff --git a/libs/ardour/ardour/audioregion.h b/libs/ardour/ardour/audioregion.h
index 45b7fea520..2d74d91482 100644
--- a/libs/ardour/ardour/audioregion.h
+++ b/libs/ardour/ardour/audioregion.h
@@ -167,10 +167,10 @@ class LIBARDOUR_API AudioRegion : public Region
 
 	void add_transient (framepos_t where);
 	void remove_transient (framepos_t where);
-	int set_transients (AnalysisFeatureList&);
-	int get_transients (AnalysisFeatureList&, bool force_new = false);
-	int update_transient (framepos_t old_position, framepos_t new_position);
-	int adjust_transients (frameoffset_t delta);
+	void clear_transients ();
+	void set_onsets (AnalysisFeatureList&);
+	void get_transients (AnalysisFeatureList&);
+	void update_transient (framepos_t old_position, framepos_t new_position);
 
 	AudioIntervalResult find_silence (Sample, framecnt_t, framecnt_t, InterThreadInfo&) const;
 
@@ -188,6 +188,8 @@ class LIBARDOUR_API AudioRegion : public Region
 	friend class ::AudioRegionReadTest;
 	friend class ::PlaylistReadTest;
 
+	void build_transients ();
+
 	PBD::Property<bool>     _envelope_active;
 	PBD::Property<bool>     _default_fade_in;
 	PBD::Property<bool>     _default_fade_out;
diff --git a/libs/ardour/ardour/region.h b/libs/ardour/ardour/region.h
index 9df4bdee7a..6839340fee 100644
--- a/libs/ardour/ardour/region.h
+++ b/libs/ardour/ardour/region.h
@@ -101,8 +101,6 @@ class LIBARDOUR_API Region
 
 	const DataType& data_type () const { return _type; }
 
-	AnalysisFeatureList transients () { return _transients; };
-
 	/** How the region parameters play together:
 	 *
 	 * POSITION: first frame of the region along the timeline
@@ -163,7 +161,6 @@ class LIBARDOUR_API Region
 	bool locked ()           const { return _locked; }
 	bool position_locked ()  const { return _position_locked; }
 	bool video_locked ()     const { return _video_locked; }
-	bool valid_transients () const { return _valid_transients; }
 	bool automatic ()        const { return _automatic; }
 	bool whole_file ()       const { return _whole_file; }
 	bool captured ()         const { return !(_import || _external); }
@@ -290,37 +287,45 @@ class LIBARDOUR_API Region
 		// no transients, but its OK
 	}
 
-	virtual int update_transient (framepos_t /* old_position */, framepos_t /* new_position */) {
+	virtual void clear_transients () {
 		// no transients, but its OK
-		return 0;
 	}
 
-	virtual void remove_transient (framepos_t /* where */) {
+	virtual void update_transient (framepos_t /* old_position */, framepos_t /* new_position */) {
 		// no transients, but its OK
 	}
 
-	virtual int set_transients (AnalysisFeatureList&) {
+	virtual void remove_transient (framepos_t /* where */) {
 		// no transients, but its OK
-		return 0;
 	}
 
-	virtual int get_transients (AnalysisFeatureList&, bool force_new = false) {
-		(void) force_new;
+	virtual void set_onsets (AnalysisFeatureList&) {
 		// no transients, but its OK
-		return 0;
 	}
 
-	virtual int adjust_transients (frameoffset_t /*delta*/) {
+	/** merges _onsets and _user_transients into given list
+	 * and removed exact duplicates.
+	 */
+	void transients (AnalysisFeatureList&);
+
+	/** merges _onsets OR _transients with _user_transients into given list
+	 * if _onsets and _transients are unset, run analysis.
+	 * list is not thinned, duplicates remain in place.
+	 * 
+	 * intended for: Playlist::find_next_transient ()
+	 */
+	virtual void get_transients (AnalysisFeatureList&) {
 		// no transients, but its OK
-		return 0;
 	}
 
+	bool has_transients () const;
+
 	virtual int separate_by_channel (ARDOUR::Session&,
 			std::vector< boost::shared_ptr<Region> >&) const {
 		return 0;
 	}
 
-	void invalidate_transients ();
+	void maybe_invalidate_transients ();
 
 	void drop_sources ();
 
@@ -371,10 +376,21 @@ class LIBARDOUR_API Region
 	/** Used when timefx are applied, so we can always use the original source */
 	SourceList              _master_sources;
 
-	AnalysisFeatureList     _transients;
-
 	boost::weak_ptr<ARDOUR::Playlist> _playlist;
 
+	void merge_features (AnalysisFeatureList&, const AnalysisFeatureList&, const frameoffset_t) const;
+
+	AnalysisFeatureList     _onsets; // used by the Ferret (Aubio OnsetDetector)
+
+	// _transient_user_start is covered by  _valid_transients
+	AnalysisFeatureList     _user_transients; // user added
+	framepos_t              _transient_user_start; // region's _start relative to user_transients
+
+	// these are used by Playlist::find_next_transient() in absence of onsets
+	AnalysisFeatureList     _transients; // Source Analysis (QM Transient), user read-only
+	framepos_t              _transient_analysis_start;
+	framepos_t              _transient_analysis_end;
+
   private:
 	void mid_thaw (const PBD::PropertyChange&);
 
diff --git a/libs/ardour/audioregion.cc b/libs/ardour/audioregion.cc
index 6b549b3d13..22c1758ef1 100644
--- a/libs/ardour/audioregion.cc
+++ b/libs/ardour/audioregion.cc
@@ -213,7 +213,7 @@ AudioRegion::register_properties ()
 	, _fade_in (Properties::fade_in, boost::shared_ptr<AutomationList> (new AutomationList (*other->_fade_in.val()))) \
 	, _inverse_fade_in (Properties::fade_in, boost::shared_ptr<AutomationList> (new AutomationList (*other->_inverse_fade_in.val()))) \
 	, _fade_out (Properties::fade_in, boost::shared_ptr<AutomationList> (new AutomationList (*other->_fade_out.val()))) \
-	, _inverse_fade_out (Properties::fade_in, boost::shared_ptr<AutomationList> (new AutomationList (*other->_inverse_fade_out.val())))
+	, _inverse_fade_out (Properties::fade_in, boost::shared_ptr<AutomationList> (new AutomationList (*other->_inverse_fade_out.val()))) \
 /* a Session will reset these to its chosen defaults by calling AudioRegion::set_default_fade() */
 
 void
@@ -372,7 +372,7 @@ void
 AudioRegion::connect_to_analysis_changed ()
 {
 	for (SourceList::const_iterator i = _sources.begin(); i != _sources.end(); ++i) {
-		(*i)->AnalysisChanged.connect_same_thread (*this, boost::bind (&AudioRegion::invalidate_transients, this));
+		(*i)->AnalysisChanged.connect_same_thread (*this, boost::bind (&AudioRegion::maybe_invalidate_transients, this));
 	}
 }
 
@@ -1594,93 +1594,136 @@ AudioRegion::get_related_audio_file_channel_count () const
     return chan_count;
 }
 
-int
-AudioRegion::adjust_transients (frameoffset_t delta)
+void
+AudioRegion::clear_transients () // yet unused
 {
-	for (AnalysisFeatureList::iterator x = _transients.begin(); x != _transients.end(); ++x) {
-		(*x) = (*x) + delta;
-	}
-
+	_user_transients.clear ();
+	_valid_transients = false;
 	send_change (PropertyChange (Properties::valid_transients));
-
-	return 0;
 }
 
-int
-AudioRegion::update_transient (framepos_t old_position, framepos_t new_position)
+void
+AudioRegion::add_transient (framepos_t where)
 {
-	for (AnalysisFeatureList::iterator x = _transients.begin(); x != _transients.end(); ++x) {
-		if ((*x) == old_position) {
-			(*x) = new_position;
-			send_change (PropertyChange (Properties::valid_transients));
+	if (where < first_frame () || where >= last_frame ()) {
+		return;
+	}
+	where -= _position;
 
-			break;
+	if (!_valid_transients) {
+		_transient_user_start = _start;
+		_valid_transients = true;
+	}
+	frameoffset_t offset = _transient_user_start - _start;
+
+	if (where < offset) {
+		if (offset <= 0) {
+			return;
+		}
+		// region start changed (extend to front), shift points and offset
+		for (AnalysisFeatureList::iterator x = _transients.begin(); x != _transients.end(); ++x) {
+			(*x) += offset;
 		}
+		_transient_user_start -= offset;
+		offset = 0;
 	}
 
-	return 0;
+	const framepos_t p = where - offset;
+	_user_transients.push_back(p);
+	send_change (PropertyChange (Properties::valid_transients));
 }
 
 void
-AudioRegion::add_transient (framepos_t where)
+AudioRegion::update_transient (framepos_t old_position, framepos_t new_position)
 {
-	_transients.push_back(where);
-	_valid_transients = true;
+	bool changed = false;
+	if (!_onsets.empty ()) {
+		const framepos_t p = old_position - _position;
+		AnalysisFeatureList::iterator x = std::find (_onsets.begin (), _onsets.end (), p);
+		if (x != _transients.end ()) {
+			(*x) = new_position - _position;
+			changed = true;
+		}
+	}
 
-	send_change (PropertyChange (Properties::valid_transients));
+	if (_valid_transients) {
+		const frameoffset_t offset = _position + _transient_user_start - _start;
+		const framepos_t p = old_position - offset;
+		AnalysisFeatureList::iterator x = std::find (_user_transients.begin (), _user_transients.end (), p);
+		if (x != _transients.end ()) {
+			(*x) = new_position - offset;
+			changed = true;
+		}
+	}
+
+	if (changed) {
+		send_change (PropertyChange (Properties::valid_transients));
+	}
 }
 
 void
 AudioRegion::remove_transient (framepos_t where)
 {
-	_transients.remove(where);
-	_valid_transients = true;
+	bool changed = false;
+	if (!_onsets.empty ()) {
+		const framepos_t p = where - _position;
+		AnalysisFeatureList::iterator i = std::find (_onsets.begin (), _onsets.end (), p);
+		if (i != _transients.end ()) {
+			_onsets.erase (i);
+			changed = true;
+		}
+	}
 
-	send_change (PropertyChange (Properties::valid_transients));
+	if (_valid_transients) {
+		const framepos_t p = where - (_position + _transient_user_start - _start);
+		AnalysisFeatureList::iterator i = std::find (_user_transients.begin (), _user_transients.end (), p);
+		if (i != _transients.end ()) {
+			_transients.erase (i);
+			changed = true;
+		}
+	}
+
+	if (changed) {
+		send_change (PropertyChange (Properties::valid_transients));
+	}
 }
 
-int
-AudioRegion::set_transients (AnalysisFeatureList& results)
+void
+AudioRegion::set_onsets (AnalysisFeatureList& results)
 {
-	_transients.clear();
-	_transients = results;
-	_valid_transients = true;
-
+	_onsets.clear();
+	_onsets = results;
 	send_change (PropertyChange (Properties::valid_transients));
-
-	return 0;
 }
 
-int
-AudioRegion::get_transients (AnalysisFeatureList& results, bool force_new)
+void
+AudioRegion::build_transients ()
 {
+	_transients.clear ();
+	_transient_analysis_start = _transient_analysis_end = 0;
+
 	boost::shared_ptr<Playlist> pl = playlist();
 
 	if (!pl) {
-		return -1;
-	}
-
-	if (_valid_transients && !force_new) {
-		results = _transients;
-		return 0;
+		return;
 	}
 
+	/* check analyzed sources first */
 	SourceList::iterator s;
-
 	for (s = _sources.begin() ; s != _sources.end(); ++s) {
 		if (!(*s)->has_been_analysed()) {
+#ifndef NDEBUG
 			cerr << "For " << name() << " source " << (*s)->name() << " has not been analyzed\n";
+#endif
 			break;
 		}
 	}
 
 	if (s == _sources.end()) {
 		/* all sources are analyzed, merge data from each one */
-
 		for (s = _sources.begin() ; s != _sources.end(); ++s) {
 
 			/* find the set of transients within the bounds of this region */
-
 			AnalysisFeatureList::iterator low = lower_bound ((*s)->transients.begin(),
 									 (*s)->transients.end(),
 									 _start);
@@ -1690,23 +1733,19 @@ AudioRegion::get_transients (AnalysisFeatureList& results, bool force_new)
 									  _start + _length);
 
 			/* and add them */
-
-			results.insert (results.end(), low, high);
+			_transients.insert (_transients.end(), low, high);
 		}
 
-		TransientDetector::cleanup_transients (results, pl->session().frame_rate(), 3.0);
+		TransientDetector::cleanup_transients (_transients, pl->session().frame_rate(), 3.0);
 
 		/* translate all transients to current position */
-
-		for (AnalysisFeatureList::iterator x = results.begin(); x != results.end(); ++x) {
+		for (AnalysisFeatureList::iterator x = _transients.begin(); x != _transients.end(); ++x) {
 			(*x) -= _start;
-			(*x) += _position;
 		}
 
-		_transients = results;
-		_valid_transients = true;
-
-		return 0;
+		_transient_analysis_start = _start;
+		_transient_analysis_end = _start + _length;
+		return;
 	}
 
 	/* no existing/complete transient info */
@@ -1721,7 +1760,7 @@ You currently have \"auto-analyse-audio\" disabled, which means \
 that transient data must be generated every time it is required.\n\n\
 If you are doing work that will require transient data on a \
 regular basis, you should probably enable \"auto-analyse-audio\" \
-then quit %1 and restart.\n\n\
+in Preferences > Audio > Regions, then quit %1 and restart.\n\n\
 This dialog will not display again.  But you may notice a slight delay \
 in this and future transient-detection operations.\n\
 "), PROGRAM_NAME));
@@ -1729,64 +1768,74 @@ in this and future transient-detection operations.\n\
 		}
 	}
 
-	bool existing_results = !results.empty();
-
 	try {
-
 		TransientDetector t (pl->session().frame_rate());
-
-		_transients.clear ();
-		_valid_transients = false;
-
 		for (uint32_t i = 0; i < n_channels(); ++i) {
 
 			AnalysisFeatureList these_results;
 
 			t.reset ();
 
+			/* this produces analysis result relative to current position
+			 * ::read() sample 0 is at _position */
 			if (t.run ("", this, i, these_results)) {
-				return -1;
-			}
-
-			/* translate all transients to give absolute position */
-
-			for (AnalysisFeatureList::iterator i = these_results.begin(); i != these_results.end(); ++i) {
-				(*i) += _position;
+				return;
 			}
 
 			/* merge */
-
 			_transients.insert (_transients.end(), these_results.begin(), these_results.end());
 		}
 	} catch (...) {
 		error << string_compose(_("Transient Analysis failed for %1."), _("Audio Region")) << endmsg;
-		return -1;
+		return;
 	}
 
-	if (!results.empty()) {
-		if (existing_results) {
-
-			/* merge our transients into the existing ones, then clean up
-			   those.
-			*/
-
-			results.insert (results.end(), _transients.begin(), _transients.end());
-			TransientDetector::cleanup_transients (results, pl->session().frame_rate(), 3.0);
-		}
-
-		/* make sure ours are clean too */
+	TransientDetector::cleanup_transients (_transients, pl->session().frame_rate(), 3.0);
+	_transient_analysis_start = _start;
+	_transient_analysis_end = _start + _length;
+}
 
-		TransientDetector::cleanup_transients (_transients, pl->session().frame_rate(), 3.0);
+/* Transient analysis uses ::read() which is relative to _start,
+ * at the time of analysis and spans _length samples.
+ *
+ * This is true for RhythmFerret::run_analysis and the
+ * TransientDetector here.
+ *
+ * We store _start and length in _transient_analysis_start,
+ * _transient_analysis_end in case the region is trimmed or split after analysis.
+ *
+ * Various methods (most notably Playlist::find_next_transient and
+ * RhythmFerret::do_split_action) span multiple regions and *merge/combine*
+ * Analysis results.
+ * We therefore need to translate the analysis timestamps to absolute session-time
+ * and include the _position of the region.
+ *
+ * Note: we should special case the AudioRegionView. The region-view itself
+ * is located at _position (currently ARV subtracts _position again)
+ */
+void
+AudioRegion::get_transients (AnalysisFeatureList& results)
+{
+	boost::shared_ptr<Playlist> pl = playlist();
+	if (!playlist ()) {
+		return;
+	}
 
-	} else {
+	Region::merge_features (results, _user_transients, _position + _transient_user_start - _start);
 
-		TransientDetector::cleanup_transients (_transients, pl->session().frame_rate(), 3.0);
-		results = _transients;
+	if (!_onsets.empty ()) {
+		// onsets are invalidated when start or length changes
+		merge_features (results, _onsets, _position);
+		return;
 	}
 
-	_valid_transients = true;
+	if ((_transient_analysis_start == _transient_analysis_end)
+			|| _transient_analysis_start > _start
+			|| _transient_analysis_end < _start + _length) {
+		build_transients ();
+	}
 
-	return 0;
+	merge_features (results, _transients, _position + _transient_analysis_start - _start);
 }
 
 /** Find areas of `silence' within a region.
diff --git a/libs/ardour/playlist.cc b/libs/ardour/playlist.cc
index 239622a38c..5861415773 100644
--- a/libs/ardour/playlist.cc
+++ b/libs/ardour/playlist.cc
@@ -1949,7 +1949,7 @@ Playlist::find_next_transient (framepos_t from, int dir)
 	bool reached = false;
 
 	if (dir > 0) {
-		for (AnalysisFeatureList::iterator x = points.begin(); x != points.end(); ++x) {
+		for (AnalysisFeatureList::const_iterator x = points.begin(); x != points.end(); ++x) {
 			if ((*x) >= from) {
 				reached = true;
 			}
@@ -1959,7 +1959,7 @@ Playlist::find_next_transient (framepos_t from, int dir)
 			}
 		}
 	} else {
-		for (AnalysisFeatureList::reverse_iterator x = points.rbegin(); x != points.rend(); ++x) {
+		for (AnalysisFeatureList::const_reverse_iterator x = points.rbegin(); x != points.rend(); ++x) {
 			if ((*x) <= from) {
 				reached = true;
 			}
diff --git a/libs/ardour/region.cc b/libs/ardour/region.cc
index 3bd51e1f18..753e578f64 100644
--- a/libs/ardour/region.cc
+++ b/libs/ardour/region.cc
@@ -36,6 +36,7 @@
 #include "ardour/session.h"
 #include "ardour/source.h"
 #include "ardour/tempo.h"
+#include "ardour/transient_detector.h"
 
 #include "i18n.h"
 
@@ -171,6 +172,9 @@ Region::register_properties ()
 	, _length (Properties::length, (l))	\
 	, _position (Properties::position, 0) \
 	, _sync_position (Properties::sync_position, (s)) \
+	, _transient_user_start (0) \
+	, _transient_analysis_start (0) \
+	, _transient_analysis_end (0) \
 	, _muted (Properties::muted, false) \
 	, _opaque (Properties::opaque, true) \
 	, _locked (Properties::locked, false) \
@@ -197,7 +201,12 @@ Region::register_properties ()
 	, _length(Properties::length, other->_length)		\
 	, _position(Properties::position, other->_position)	\
 	, _sync_position(Properties::sync_position, other->_sync_position) \
-        , _muted (Properties::muted, other->_muted)	        \
+	, _user_transients (other->_user_transients) \
+	, _transient_user_start (other->_transient_user_start) \
+	, _transients (other->_transients) \
+	, _transient_analysis_start (other->_transient_analysis_start) \
+	, _transient_analysis_end (other->_transient_analysis_end) \
+	, _muted (Properties::muted, other->_muted)	        \
 	, _opaque (Properties::opaque, other->_opaque)		\
 	, _locked (Properties::locked, other->_locked)		\
   , _video_locked (Properties::video_locked, other->_video_locked) \
@@ -434,7 +443,7 @@ Region::set_length (framecnt_t len)
 		_whole_file = false;
 		first_edit ();
 		maybe_uncopy ();
-		invalidate_transients ();
+		maybe_invalidate_transients ();
 
 		if (!property_changes_suspended()) {
 			recompute_at_end ();
@@ -635,8 +644,6 @@ Region::set_position_internal (framepos_t pos, bool allow_bbt_recompute)
 		if (allow_bbt_recompute) {
 			recompute_position_from_lock_style ();
 		}
-
-		//invalidate_transients ();
 	}
 }
 
@@ -709,7 +716,7 @@ Region::set_start (framepos_t pos)
 		set_start_internal (pos);
 		_whole_file = false;
 		first_edit ();
-		invalidate_transients ();
+		maybe_invalidate_transients ();
 
 		send_change (Properties::start);
 	}
@@ -797,7 +804,6 @@ Region::modify_front (framepos_t new_position, bool reset_fade)
 	if (new_position < end) { /* can't trim it zero or negative length */
 
 		framecnt_t newlen = 0;
-		framepos_t delta = 0;
 
 		if (!can_trim_start_before_source_start ()) {
 			/* can't trim it back past where source position zero is located */
@@ -806,10 +812,8 @@ Region::modify_front (framepos_t new_position, bool reset_fade)
 
 		if (new_position > _position) {
 			newlen = _length - (new_position - _position);
-			delta = -1 * (new_position - _position);
 		} else {
 			newlen = _length + (_position - new_position);
-			delta = _position - new_position;
 		}
 
 		trim_to_internal (new_position, newlen);
@@ -822,9 +826,7 @@ Region::modify_front (framepos_t new_position, bool reset_fade)
 			recompute_at_start ();
 		}
 
-		if (_transients.size() > 0){
-			adjust_transients(delta);
-		}
+		maybe_invalidate_transients ();
 	}
 }
 
@@ -1303,7 +1305,7 @@ Region::_set_state (const XMLNode& node, int /*version*/, PropertyChange& what_c
 	}
 
 	// saved property is invalid, region-transients are not saved
-	if (_transients.size() == 0){
+	if (_user_transients.size() == 0){
 		_valid_transients = false;
 	}
 
@@ -1627,12 +1629,60 @@ Region::apply (Filter& filter, Progress* progress)
 
 
 void
-Region::invalidate_transients ()
+Region::maybe_invalidate_transients ()
+{
+	bool changed = !_onsets.empty();
+	_onsets.clear ();
+
+	if (_valid_transients || changed) {
+		send_change (PropertyChange (Properties::valid_transients));
+		return;
+	}
+}
+
+void
+Region::transients (AnalysisFeatureList& afl)
 {
-	_valid_transients = false;
-	_transients.clear ();
+	int cnt = afl.empty() ? 0 : 1;
+
+	Region::merge_features (afl, _onsets, _position);
+	Region::merge_features (afl, _user_transients, _position + _transient_user_start - _start);
+	if (!_onsets.empty ()) {
+		++cnt;
+	}
+	if (!_user_transients.empty ()) {
+		++cnt;
+	}
+	if (cnt > 1 ) {
+		afl.sort ();
+		// remove exact duplicates
+		TransientDetector::cleanup_transients (afl, _session.frame_rate(), 0);
+	}
+}
 
-	send_change (PropertyChange (Properties::valid_transients));
+bool
+Region::has_transients () const
+{
+	if (!_user_transients.empty ()) {
+		assert (_valid_transients);
+		return true;
+	}
+	if (!_onsets.empty ()) {
+		return true;
+	}
+	return false;
+}
+
+void
+Region::merge_features (AnalysisFeatureList& result, const AnalysisFeatureList& src, const frameoffset_t off) const
+{
+	for (AnalysisFeatureList::const_iterator x = src.begin(); x != src.end(); ++x) {
+		const frameoffset_t p = (*x) + off;
+		if (p < first_frame() || p > last_frame()) {
+			continue;
+		}
+		result.push_back (p);
+	}
 }
 
 void
diff --git a/libs/ardour/transient_detector.cc b/libs/ardour/transient_detector.cc
index 41bb51f710..15b2a4ce08 100644
--- a/libs/ardour/transient_detector.cc
+++ b/libs/ardour/transient_detector.cc
@@ -119,7 +119,7 @@ TransientDetector::cleanup_transients (AnalysisFeatureList& t, float sr, float g
 
 		// move f until we find a new value that is far enough away
 
-		while ((f != t.end()) && (((*f) - (*i)) < gap_frames)) {
+		while ((f != t.end()) && gap_frames > 0 && (((*f) - (*i)) < gap_frames)) {
 			++f;
 		}