Make some musical operations on music-locked regions operate in beats.

	- use exact beats to determine frame position.
	- see comments in tempo.cc for more.
	- this hasn't been done for split yet, but dragging and
	  trimming are supported.

6 files changed, 123 insertions, 58 deletions

diff --git a/libs/ardour/ardour/midi_region.h b/libs/ardour/ardour/midi_region.h
index b4557ed1dd..3a097c907e 100644
--- a/libs/ardour/ardour/midi_region.h
+++ b/libs/ardour/ardour/midi_region.h
@@ -133,8 +133,8 @@ class LIBARDOUR_API MidiRegion : public Region
 
 	void set_position_internal (framepos_t pos, bool allow_bbt_recompute);
 	void set_length_internal (framecnt_t len);
-	void set_start_internal (framecnt_t);
-	void trim_to_internal (framepos_t position, framecnt_t length);
+	void set_start_internal (framecnt_t, const int32_t& sub_num);
+	void trim_to_internal (framepos_t position, framecnt_t length, const int32_t& sub_num);
 	void update_length_beats ();
 
 	void model_changed ();
diff --git a/libs/ardour/ardour/region.h b/libs/ardour/ardour/region.h
index 776a8a7966..e0dd159ce5 100644
--- a/libs/ardour/ardour/region.h
+++ b/libs/ardour/ardour/region.h
@@ -207,7 +207,7 @@ class LIBARDOUR_API Region
 
 	void set_length (framecnt_t);
 	void set_start (framepos_t);
-	void set_position (framepos_t);
+	void set_position (framepos_t, int32_t sub_num = 0);
 	void set_initial_position (framepos_t);
 	void special_set_position (framepos_t);
 	virtual void update_after_tempo_map_change (bool send_change = true);
@@ -216,15 +216,15 @@ class LIBARDOUR_API Region
 	bool at_natural_position () const;
 	void move_to_natural_position ();
 
-	void move_start (frameoffset_t distance);
-	void trim_front (framepos_t new_position);
-	void trim_end (framepos_t new_position);
-	void trim_to (framepos_t position, framecnt_t length);
+	void move_start (frameoffset_t distance, const int32_t& sub_num = 0);
+	void trim_front (framepos_t new_position, const int32_t& sub_num = 0);
+	void trim_end (framepos_t new_position, const int32_t& sub_num = 0);
+	void trim_to (framepos_t position, framecnt_t length, const int32_t& sub_num = 0);
 
 	virtual void fade_range (framepos_t, framepos_t) {}
 
-	void cut_front (framepos_t new_position);
-	void cut_end (framepos_t new_position);
+	void cut_front (framepos_t new_position, const int32_t& sub_num = 0);
+	void cut_end (framepos_t new_position, const int32_t& sub_num = 0);
 
 	void set_layer (layer_t l); /* ONLY Playlist can call this */
 	void raise ();
@@ -358,7 +358,7 @@ class LIBARDOUR_API Region
 	void post_set (const PBD::PropertyChange&);
 	virtual void set_position_internal (framepos_t pos, bool allow_bbt_recompute);
 	virtual void set_length_internal (framecnt_t);
-	virtual void set_start_internal (framecnt_t);
+	virtual void set_start_internal (framecnt_t, const int32_t& sub_num = 0);
 	bool verify_start_and_length (framepos_t, framecnt_t&);
 	void first_edit ();
 
@@ -396,9 +396,9 @@ class LIBARDOUR_API Region
   private:
 	void mid_thaw (const PBD::PropertyChange&);
 
-	virtual void trim_to_internal (framepos_t position, framecnt_t length);
-	void modify_front (framepos_t new_position, bool reset_fade);
-	void modify_end (framepos_t new_position, bool reset_fade);
+	virtual void trim_to_internal (framepos_t position, framecnt_t length, const int32_t& sub_num);
+	void modify_front (framepos_t new_position, bool reset_fade, const int32_t& sub_num);
+	void modify_end (framepos_t new_position, bool reset_fade, const int32_t& sub_num);
 
 	void maybe_uncopy ();
 
diff --git a/libs/ardour/ardour/tempo.h b/libs/ardour/ardour/tempo.h
index 33d9fb937d..1bf4c47d7a 100644
--- a/libs/ardour/ardour/tempo.h
+++ b/libs/ardour/ardour/tempo.h
@@ -450,6 +450,8 @@ class LIBARDOUR_API TempoMap : public PBD::StatefulDestructible
 	bool gui_change_tempo (TempoSection*, const Tempo& bpm);
 	void gui_dilate_tempo (TempoSection* tempo, const framepos_t& frame, const framepos_t& end_frame, const double& pulse);
 
+	double exact_beat_at_frame (const framepos_t& frame, const int32_t& sub_num);
+
 	std::pair<double, framepos_t> predict_tempo_position (TempoSection* section, const Timecode::BBT_Time& bbt);
 	bool can_solve_bbt (TempoSection* section, const Timecode::BBT_Time& bbt);
 
@@ -493,6 +495,8 @@ private:
 	bool solve_map_frame (Metrics& metrics, MeterSection* section, const framepos_t& frame);
 	bool solve_map_bbt (Metrics& metrics, MeterSection* section, const Timecode::BBT_Time& bbt);
 
+	double exact_beat_at_frame_locked (const Metrics& metrics, const framepos_t& frame, const int32_t& sub_num);
+
 	friend class ::BBTTest;
 	friend class ::FrameposPlusBeatsTest;
 	friend class ::TempoTest;
diff --git a/libs/ardour/midi_region.cc b/libs/ardour/midi_region.cc
index 81bfc2aa23..26a8509aca 100644
--- a/libs/ardour/midi_region.cc
+++ b/libs/ardour/midi_region.cc
@@ -458,14 +458,17 @@ MidiRegion::fix_negative_start ()
 }
 
 void
-MidiRegion::set_start_internal (framecnt_t s)
+MidiRegion::set_start_internal (framecnt_t s, const int32_t& sub_num)
 {
-	Region::set_start_internal (s);
-	set_start_beats_from_start_frames ();
+	Region::set_start_internal (s, sub_num);
+
+	if (position_lock_style() == AudioTime) {
+		set_start_beats_from_start_frames ();
+	}
 }
 
 void
-MidiRegion::trim_to_internal (framepos_t position, framecnt_t length)
+MidiRegion::trim_to_internal (framepos_t position, framecnt_t length, const int32_t& sub_num)
 {
 	framepos_t new_start;
 
@@ -476,7 +479,7 @@ MidiRegion::trim_to_internal (framepos_t position, framecnt_t length)
 	PropertyChange what_changed;
 
 	/* beat has not been set by set_position_internal */
-	const double beat_delta = _session.tempo_map().beat_at_frame (position) - beat();
+	const double beat_delta = _session.tempo_map().exact_beat_at_frame (position, sub_num) - beat();
 
 	/* Set position before length, otherwise for MIDI regions this bad thing happens:
 	 * 1. we call set_length_internal; length in beats is computed using the region's current
@@ -504,7 +507,7 @@ MidiRegion::trim_to_internal (framepos_t position, framecnt_t length)
 		_start_beats = Evoral::Beats (new_start_beat);
 		what_changed.add (Properties::start_beats);
 
-		set_start_internal (new_start);
+		set_start_internal (new_start, sub_num);
 		what_changed.add (Properties::start);
 	}
 
diff --git a/libs/ardour/region.cc b/libs/ardour/region.cc
index ba18cbc62d..5fb5b30014 100644
--- a/libs/ardour/region.cc
+++ b/libs/ardour/region.cc
@@ -570,13 +570,19 @@ Region::update_after_tempo_map_change (bool send)
 }
 
 void
-Region::set_position (framepos_t pos)
+Region::set_position (framepos_t pos, int32_t sub_num)
 {
 	if (!can_move()) {
 		return;
 	}
 
-	set_position_internal (pos, true);
+	if (sub_num == 0) {
+		set_position_internal (pos, true);
+	} else {
+		double beat = _session.tempo_map().exact_beat_at_frame (pos, sub_num);
+		_beat = beat;
+		set_position_internal (pos, false);
+	}
 
 	/* do this even if the position is the same. this helps out
 	   a GUI that has moved its representation already.
@@ -738,7 +744,7 @@ Region::set_start (framepos_t pos)
 }
 
 void
-Region::move_start (frameoffset_t distance)
+Region::move_start (frameoffset_t distance, const int32_t& sub_num)
 {
 	if (locked() || position_locked() || video_locked()) {
 		return;
@@ -774,7 +780,7 @@ Region::move_start (frameoffset_t distance)
 		return;
 	}
 
-	set_start_internal (new_start);
+	set_start_internal (new_start, sub_num);
 
 	_whole_file = false;
 	first_edit ();
@@ -783,25 +789,25 @@ Region::move_start (frameoffset_t distance)
 }
 
 void
-Region::trim_front (framepos_t new_position)
+Region::trim_front (framepos_t new_position, const int32_t& sub_num)
 {
-	modify_front (new_position, false);
+	modify_front (new_position, false, sub_num);
 }
 
 void
-Region::cut_front (framepos_t new_position)
+Region::cut_front (framepos_t new_position, const int32_t& sub_num)
 {
-	modify_front (new_position, true);
+	modify_front (new_position, true, sub_num);
 }
 
 void
-Region::cut_end (framepos_t new_endpoint)
+Region::cut_end (framepos_t new_endpoint, const int32_t& sub_num)
 {
-	modify_end (new_endpoint, true);
+	modify_end (new_endpoint, true, sub_num);
 }
 
 void
-Region::modify_front (framepos_t new_position, bool reset_fade)
+Region::modify_front (framepos_t new_position, bool reset_fade, const int32_t& sub_num)
 {
 	if (locked()) {
 		return;
@@ -831,7 +837,7 @@ Region::modify_front (framepos_t new_position, bool reset_fade)
 			newlen = _length + (_position - new_position);
 		}
 
-		trim_to_internal (new_position, newlen);
+		trim_to_internal (new_position, newlen, sub_num);
 
 		if (reset_fade) {
 			_right_of_split = true;
@@ -846,14 +852,14 @@ Region::modify_front (framepos_t new_position, bool reset_fade)
 }
 
 void
-Region::modify_end (framepos_t new_endpoint, bool reset_fade)
+Region::modify_end (framepos_t new_endpoint, bool reset_fade, const int32_t& sub_num)
 {
 	if (locked()) {
 		return;
 	}
 
 	if (new_endpoint > _position) {
-		trim_to_internal (_position, new_endpoint - _position);
+		trim_to_internal (_position, new_endpoint - _position, sub_num);
 		if (reset_fade) {
 			_left_of_split = true;
 		}
@@ -868,19 +874,19 @@ Region::modify_end (framepos_t new_endpoint, bool reset_fade)
  */
 
 void
-Region::trim_end (framepos_t new_endpoint)
+Region::trim_end (framepos_t new_endpoint, const int32_t& sub_num)
 {
-	modify_end (new_endpoint, false);
+	modify_end (new_endpoint, false, sub_num);
 }
 
 void
-Region::trim_to (framepos_t position, framecnt_t length)
+Region::trim_to (framepos_t position, framecnt_t length, const int32_t& sub_num)
 {
 	if (locked()) {
 		return;
 	}
 
-	trim_to_internal (position, length);
+	trim_to_internal (position, length, sub_num);
 
 	if (!property_changes_suspended()) {
 		recompute_at_start ();
@@ -889,7 +895,7 @@ Region::trim_to (framepos_t position, framecnt_t length)
 }
 
 void
-Region::trim_to_internal (framepos_t position, framecnt_t length)
+Region::trim_to_internal (framepos_t position, framecnt_t length, const int32_t& sub_num)
 {
 	framepos_t new_start;
 
@@ -926,7 +932,7 @@ Region::trim_to_internal (framepos_t position, framecnt_t length)
 	PropertyChange what_changed;
 
 	if (_start != new_start) {
-		set_start_internal (new_start);
+		set_start_internal (new_start, sub_num);
 		what_changed.add (Properties::start);
 	}
 
@@ -1845,7 +1851,7 @@ Region::post_set (const PropertyChange& pc)
 }
 
 void
-Region::set_start_internal (framecnt_t s)
+Region::set_start_internal (framecnt_t s, const int32_t& sub_num)
 {
 	_start = s;
 }
diff --git a/libs/ardour/tempo.cc b/libs/ardour/tempo.cc
index c5c7e2c52f..5df06f7745 100644
--- a/libs/ardour/tempo.cc
+++ b/libs/ardour/tempo.cc
@@ -586,12 +586,16 @@ MeterSection::get_state() const
 /*
   Tempo Map Overview
 
+  The Shaggs - Things I Wonder
+  https://www.youtube.com/watch?v=9wQK6zMJOoQ
+
   Tempo is the rate of the musical pulse.
   Meters divide the pulses into measures and beats.
 
   TempoSections - provide pulses in the form of beats_per_minute() and note_type() where note_type is the division of a whole pulse,
   and beats_per_minute is the number of note_types in one minute (unlike what its name suggests).
-  Note that Tempo::beats_per_minute() has nothing to do with musical beats.
+  Note that Tempo::beats_per_minute() has nothing to do with musical beats. It has been left that way because
+  a shorter one hasn't been found yet (pulse_divisions_per_minute()?).
 
   MeterSecions - divide pulses into measures (via divisions_per_bar) and beats (via note_divisor).
 
@@ -629,6 +633,43 @@ MeterSection::get_state() const
   Because ramped MusicTime and AudioTime tempos can interact with each other,
   reordering is frequent. Care must be taken to keep _metrics in a solved state.
   Solved means ordered by frame or pulse with frame-accurate precision (see check_solved()).
+
+  Music and Audio
+
+  Music and audio-locked objects may seem interchangeable on the surface, but when translating
+  between audio samples and beats, keep in mind that a sample is only a quantised approximation
+  of the actual time (in minutes) of a beat.
+  Thus if a gui user points to the frame occupying the start of a music-locked object on 1|3|0, it does not
+  mean that this frame is the actual location in time of 1|3|0.
+
+  You cannot use a frame measurement to determine beat distance except under special circumstances
+  (e.g. where the user has requested that a beat lie on a SMPTE frame or if the tempo is known to be constant over the duration).
+
+  This means is that a user operating on a musical grid must supply the desired beat position and/or current beat quantization in order for the
+  sample space the user is operating at to be translated correctly to the object.
+
+  The current approach is to interpret the supplied frame using the grid division the user has currently selected.
+  If the user has no musical grid set, they are actually operating in sample space (even SMPTE frames are rounded to audio frame), so
+  the supplied audio frame is interpreted as the desired musical location (beat_at_frame()).
+
+  tldr: Beat, being a function of time, has nothing to do with sample rate, but time quantization can get in the way of precision.
+
+  When frame_at_beat() is called, the position calculation is performed in pulses and minutes.
+  The result is rounded to audio frames.
+  When beat_at_frame() is called, the frame is converted to minutes, with no rounding performed on the result.
+
+  So :
+  frame_at_beat (beat_at_frame (frame)) == frame
+  but :
+  beat_at_frame (frame_at_beat (beat)) != beat due to the time quantization of frame_at_beat().
+
+  Doing the second one will result in a beat distance error of up to 0.5 audio samples.
+  So instead work in pulses and/or beats and only use beat position to caclulate frame position (e.g. after tempo change).
+  For audio-locked objects, use frame position to calculate beat position.
+
+  The above pointless example would then do:
+  beat_at_pulse (pulse_at_beat (beat)) to avoid rounding.
+
 */
 struct MetricSectionSorter {
     bool operator() (const MetricSection* a, const MetricSection* b) {
@@ -2545,25 +2586,9 @@ TempoMap::gui_move_tempo (TempoSection* ts, const framepos_t& frame, const int&
 			/* if we're snapping to a musical grid, set the pulse exactly instead of via the supplied frame. */
 			Glib::Threads::RWLock::WriterLock lm (lock);
 			TempoSection* tempo_copy = copy_metrics_and_point (_metrics, future_map, ts);
-			double beat = beat_at_frame_locked (future_map, frame);
-
-			if (sub_num > 1) {
-				beat = floor (beat) + (floor (((beat - floor (beat)) * (double) sub_num) + 0.5) / sub_num);
-			} else if (sub_num == 1) {
-				/* snap to beat */
-				beat = floor (beat + 0.5);
-			}
-
+			const double beat = exact_beat_at_frame_locked (future_map, frame, sub_num);
 			double pulse = pulse_at_beat_locked (future_map, beat);
 
-			if (sub_num == -1) {
-				/* snap to  bar */
-				BBT_Time bbt = bbt_at_beat_locked (future_map, beat);
-				bbt.beats = 1;
-				bbt.ticks = 0;
-				pulse = pulse_at_bbt_locked (future_map, bbt);
-			}
-
 			if (solve_map_pulse (future_map, tempo_copy, pulse)) {
 				solve_map_pulse (_metrics, ts, pulse);
 				recompute_meters (_metrics);
@@ -2811,6 +2836,33 @@ TempoMap::gui_dilate_tempo (TempoSection* ts, const framepos_t& frame, const fra
 	MetricPositionChanged (); // Emit Signal
 }
 
+double
+TempoMap::exact_beat_at_frame (const framepos_t& frame, const int32_t& sub_num)
+{
+	Glib::Threads::RWLock::ReaderLock lm (lock);
+
+	return exact_beat_at_frame_locked (_metrics, frame, sub_num);
+}
+
+double
+TempoMap::exact_beat_at_frame_locked (const Metrics& metrics, const framepos_t& frame, const int32_t& sub_num)
+{
+	double beat = beat_at_frame_locked (metrics, frame);
+	if (sub_num > 1) {
+		beat = floor (beat) + (floor (((beat - floor (beat)) * (double) sub_num) + 0.5) / sub_num);
+	} else if (sub_num == 1) {
+		/* snap to beat */
+		beat = floor (beat + 0.5);
+	} else if (sub_num == -1) {
+		/* snap to  bar */
+		Timecode::BBT_Time bbt = bbt_at_beat_locked (metrics, beat);
+		bbt.beats = 1;
+		bbt.ticks = 0;
+		beat = beat_at_bbt_locked (metrics, bbt);
+	}
+	return beat;
+}
+
 framecnt_t
 TempoMap::bbt_duration_at (framepos_t pos, const BBT_Time& bbt, int dir)
 {