From 434f460775e49ad7a59ad3d27617b175ccf269a6 Mon Sep 17 00:00:00 2001
From: Paul Davis <paul@linuxaudiosystems.com>
Date: Fri, 28 Feb 2025 13:39:16 -0700
Subject: [PATCH] introduce Region::ensure_length_sanity()

This forces the length of AudioRegions to be an integer number of samples. It is intended
to fix a set of bugs that occur when using music time as the session time domain
and carrying out editing operations that would otherwise lead to audio regions
whose length involves fractional samples.

It is perfectly legal to specific audio distances that include fractional samples,
but there is no reason for any audio region to ever have such a length (we think).
---
 libs/ardour/ardour/audioregion.h |  1 +
 libs/ardour/ardour/region.h      |  1 +
 libs/ardour/audioregion.cc       | 18 ++++++++++++++
 libs/ardour/region.cc            | 42 +++++++++++++++++++++-----------
 4 files changed, 48 insertions(+), 14 deletions(-)

diff --git a/libs/ardour/ardour/audioregion.h b/libs/ardour/ardour/audioregion.h
index 07a6e99dec..317363182d 100644
--- a/libs/ardour/ardour/audioregion.h
+++ b/libs/ardour/ardour/audioregion.h
@@ -289,6 +289,7 @@ class LIBARDOUR_API AudioRegion : public Region, public AudioReadable
 
 	int _set_state (const XMLNode&, int version, PBD::PropertyChange& what_changed, bool send_signal);
 	void send_change (const PBD::PropertyChange&);
+	void ensure_length_sanity ();
 };
 
 } /* namespace ARDOUR */
diff --git a/libs/ardour/ardour/region.h b/libs/ardour/ardour/region.h
index df7b83484a..5d3c9fe39f 100644
--- a/libs/ardour/ardour/region.h
+++ b/libs/ardour/ardour/region.h
@@ -573,6 +573,7 @@ protected:
 	virtual void set_start_internal (timepos_t const &);
 	bool verify_start_and_length (timepos_t const &, timecnt_t&);
 	void first_edit ();
+	virtual void ensure_length_sanity () {}
 
 	void override_opaqueness (bool yn) {
 		_opaque = yn;
diff --git a/libs/ardour/audioregion.cc b/libs/ardour/audioregion.cc
index 4a7e74432e..e90dee6688 100644
--- a/libs/ardour/audioregion.cc
+++ b/libs/ardour/audioregion.cc
@@ -438,6 +438,8 @@ AudioRegion::~AudioRegion ()
 void
 AudioRegion::post_set (const PropertyChange& /*ignored*/)
 {
+	ensure_length_sanity ();
+
 	if (!_sync_marked) {
 		_sync_position = _start;
 	}
@@ -2701,3 +2703,19 @@ AudioRegion::apply_region_fx (BufferSet& bufs, samplepos_t start_sample, samplep
 	_fx_pos = end_sample;
 	_fx_latent_read = false;
 }
+
+void
+AudioRegion::ensure_length_sanity ()
+{
+	if (_type == DataType::AUDIO) {
+		/* Force audio regions to have a length that is the
+		   rounded-down integer number of samples. No other value makes
+		   any sort of logical sense. We tried to fix this at a lower
+		   level, by rounding the return value of
+		   TempoMap::superclock_at(), but the breaks the fundamental
+		   point of a high resolution audio time unit.
+		*/
+		_length = timecnt_t (timepos_t (_length.val().samples()), _length.val().position());
+	}
+}
+
diff --git a/libs/ardour/region.cc b/libs/ardour/region.cc
index 9d6fea03cb..dd2c179d91 100644
--- a/libs/ardour/region.cc
+++ b/libs/ardour/region.cc
@@ -304,6 +304,7 @@ Region::Region (Session& s, timepos_t const & start, timecnt_t const & length, c
 	, _changemap (0)
 {
 	register_properties ();
+	ensure_length_sanity ();
 
 	/* no sources at this point */
 }
@@ -324,6 +325,7 @@ Region::Region (const SourceList& srcs)
 	register_properties ();
 
 	use_sources (srcs);
+	ensure_length_sanity ();
 
 	assert(_sources.size() > 0);
 	assert (_type == srcs.front()->type());
@@ -382,6 +384,7 @@ Region::Region (std::shared_ptr<const Region> other)
 		_sync_position = _start;
 	}
 
+	ensure_length_sanity ();
 	assert (_type == other->data_type());
 }
 
@@ -433,6 +436,7 @@ Region::Region (std::shared_ptr<const Region> other, timecnt_t const & offset)
 		_sync_position = _start;
 	}
 
+	ensure_length_sanity ();
 	assert (_type == other->data_type());
 }
 
@@ -462,6 +466,7 @@ Region::Region (std::shared_ptr<const Region> other, const SourceList& srcs)
 	}
 
 	use_sources (srcs);
+	ensure_length_sanity ();
 	assert(_sources.size() > 0);
 }
 
@@ -567,25 +572,34 @@ Region::set_length_internal (timecnt_t const & len)
 
 	_last_length = timecnt_t (_length.val().distance(), _last_length.position());
 
-	std::shared_ptr<Playlist> pl (playlist());
+	if (_type == DataType::AUDIO) {
 
-	if (pl) {
-		Temporal::TimeDomain td (pl->time_domain());
+		/* Equivalent to what AudioRegion::ensure_length_sanity() does */
+		_length = timecnt_t (timepos_t (len.samples()), _length.val().position());
 
-		/* Note: timecnt_t::time_domain() returns the domain for the
-		 * length component, *not* the position.
-		 */
+	} else {
 
-		if (td != len.time_domain()) {
-			timecnt_t l = _length.val();
-			l.set_time_domain (td);
-			_length = l;
-			return;
+		std::shared_ptr<Playlist> pl (playlist());
+
+		if (pl) {
+			Temporal::TimeDomain td (pl->time_domain());
+
+			/* Note: timecnt_t::time_domain() returns the domain for the
+			 * length component, *not* the position.
+			 */
+
+			if (td != len.time_domain()) {
+				timecnt_t l = _length.val();
+				l.set_time_domain (td);
+				_length = l;
+				return;
+			}
 		}
-	}
-	/* either no playlist or time domain for distance is not changing */
 
-	_length = timecnt_t (len.distance(), _length.val().position());
+		/* either no playlist or time domain for distance is not changing */
+
+		_length = timecnt_t (len.distance(), _length.val().position());
+	}
 }
 
 void