View Issue Details

IDProjectCategoryView StatusLast Update
0001742ardourbugspublic2020-04-19 20:12
Reporterlincoln Assigned Topaul  
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version2.0 
Summary0001742: Latency over-compensation when recording hydrogen drums
DescriptionWhen recording drums from hydrogen I noticed that the drums were eing recorded ahead. I checked the numer of frames by which the recording was ahead and it became clear the the number of frames involved are equal to jack's stated latency.

It seems that the latency compensation is over compensating here.
TagsNo tags attached.

Activities

torbenh3

2007-07-17 22:45

reporter   ~0004150

i have spent half a day searching for mistakes in the latency compensation.
i did not find any.

but when uncommenting the slave_follow debug output:

libs/ardour/session_process.cc:468 #if 1

i get this:
delta = 1024 speed = 1 ts = 1 M@ 165888 S@ 164864 avgdelta = 1800

ardour NEVER locks onto the jack-transport.

this goes unnoticed for clients interpreting BBT correctly,
because of ardours timebase callback being somewhat misinterpreted.

i try to fix this issue...

2007-07-18 01:11

 

transport-diff.diff (2,687 bytes)   
Index: libs/ardour/ardour/audioengine.h
===================================================================
--- libs/ardour/ardour/audioengine.h	(revision 2138)
+++ libs/ardour/ardour/audioengine.h	(working copy)
@@ -84,10 +84,7 @@
 		return jack_frame_time (_jack);
 	}
 
-	nframes_t transport_frame () const {
-		if (!_running || !_jack) return 0;
-		return jack_get_current_transport_frame (_jack);
-	}
+	nframes_t transport_frame () const;
 	
 	int request_buffer_size (nframes_t);
 	
Index: libs/ardour/audioengine.cc
===================================================================
--- libs/ardour/audioengine.cc	(revision 2138)
+++ libs/ardour/audioengine.cc	(working copy)
@@ -694,6 +694,17 @@
 	}
 }
 
+
+nframes_t
+AudioEngine::transport_frame () const
+{
+    if (!_running || !_jack) return 0;
+    return jack_get_current_transport_frame (_jack);
+    //jack_position_t pos;
+    //jack_transport_query( _jack, &pos );
+    //return pos.frame;
+}
+
 Port *
 AudioEngine::get_port_by_name (const string& portname, bool keep)
 {
Index: libs/ardour/session_time.cc
===================================================================
--- libs/ardour/session_time.cc	(revision 2138)
+++ libs/ardour/session_time.cc	(working copy)
@@ -497,15 +497,15 @@
 
 	/* frame info */
 
-	pos->frame = _transport_frame;
+	//pos->frame = _transport_frame;
 	pos->valid = JackPositionTimecode;
 
 	/* BBT info */
 	
 	if (_tempo_map) {
 
-		TempoMap::Metric metric (_tempo_map->metric_at (_transport_frame));
-		_tempo_map->bbt_time_with_metric (_transport_frame, bbt, metric);
+		TempoMap::Metric metric (_tempo_map->metric_at (pos->frame));
+		_tempo_map->bbt_time_with_metric (pos->frame, bbt, metric);
 		
 		pos->bar = bbt.bars;
 		pos->beat = bbt.beats;
Index: libs/ardour/session_process.cc
===================================================================
--- libs/ardour/session_process.cc	(revision 2138)
+++ libs/ardour/session_process.cc	(working copy)
@@ -46,9 +46,7 @@
 Session::process (nframes_t nframes)
 {
 	if (synced_to_jack() && waiting_to_start) {
-		if ( _engine.transport_state() == AudioEngine::TransportRolling) {
 			actually_start_transport ();
-		}
 	}
 
 	if (non_realtime_work_pending()) {
Index: libs/ardour/session_transport.cc
===================================================================
--- libs/ardour/session_transport.cc	(revision 2138)
+++ libs/ardour/session_transport.cc	(working copy)
@@ -879,6 +879,9 @@
 void
 Session::start_transport ()
 {
+	if (synced_to_jack() && !_exporting) {
+	    _transport_frame += get_block_size();
+	}
 	_last_roll_location = _transport_frame;
 
 	/* if record status is Enabled, move it to Recording. if its
transport-diff.diff (2,687 bytes)   

torbenh3

2007-07-18 01:15

reporter   ~0004151

the attached patch removes the issue.
however ardour now start recording at period_size which is not really better.

it also contains a fix to the timebase_callback.
ardour was overwriting pos->frame which is not allowed.

the other hunk just moves a function from the headerfile to the .cc file.
i should have removed it.

torbenh3

2007-07-18 03:19

reporter   ~0004153

i have fixed the bug in jack. there might be issues with this patch,
but i think its complete.

please fix ardour timebase callback anyways. it should not be
critical anymore, but its a bad example.

2007-07-18 03:19

 

jack-transport-start-at-zero-fix.diff (1,613 bytes)   
Index: jackd/transengine.c
===================================================================
--- jackd/transengine.c	(revision 1050)
+++ jackd/transengine.c	(working copy)
@@ -395,6 +395,7 @@
 {
 	jack_control_t *ectl = engine->control;
 	transport_command_t cmd;	/* latest transport command */
+	int dont_trans_advance = FALSE;
 
 	/* Promote pending_time to current_time.  Maintain the usecs,
 	 * frame_rate and frame values, clients may not set them. */
@@ -409,6 +410,10 @@
 		if ((ectl->sync_remain == 0) ||
 		    (jack_sync_timeout(engine))) {
 			ectl->transport_state = JackTransportRolling;
+
+			// dont advance the transport this is a statechange not seen
+			// by the switch statement below
+			dont_trans_advance = TRUE;
 			VERBOSE (engine, "transport Rolling, %8.6f sec"
 				 " left for poll\n",
 				 (double) (ectl->sync_time_left / 1000000.0));
@@ -468,7 +473,12 @@
 				ectl->transport_state = JackTransportStarting;
 				jack_sync_poll_start(engine);
 			}
+		} else if ( ! dont_trans_advance ) {
+		    // ok... no statechange happened go transport go...
+		    ectl->pending_time.frame =
+			ectl->current_time.frame + ectl->buffer_size;
 		}
+
 		break;
 
 	default:
@@ -476,12 +486,6 @@
 			    ectl->transport_state);
 	}
 
-	/* Update timebase, if needed. */
-	if (ectl->transport_state == JackTransportRolling) {
-		ectl->pending_time.frame =
-			ectl->current_time.frame + ectl->buffer_size;
-	} 
-
 	/* See if an asynchronous position request arrived during the
 	 * last cycle.  The request_time could change during the
 	 * guarded copy.  If so, we use the newest request. */

torbenh3

2007-07-18 20:54

reporter   ~0004160

mhmhm... its not complete :(

lincoln

2007-08-15 12:44

reporter   ~0004268

finally managed to find time to try the patch. It now places the region late but if you snap to bar and move it everything falls into place as expected. So your patch does improve things quite a bit. It looks like the region start just needs to be offset back by 1 period_size when recording in this situation.

torbenh

2008-09-12 23:42

reporter   ~0005133

did you apply both patches ?
i had a report that only applying the jack patch fixes the problem
for hydrogen.

for muse-0.9 however it is not fixed.
but after a quick skimming over muses code i have the feeling,
that its transport code is not correct for this use case anyways.

lincoln

2008-09-13 08:00

reporter   ~0005135

Yes I only applied the jack patch and yes it does work. With the ardour patch it would make the recording late.

I have just tested this against hydrogen from svn.

2008-09-23 11:19

 

diffs.diff (876 bytes)   
Index: libs/ardour/session_process.cc
===================================================================
--- libs/ardour/session_process.cc	(revision 3794)
+++ libs/ardour/session_process.cc	(working copy)
@@ -47,6 +47,9 @@
 {
 	_silent = false;
 
+	if( !_exporting && synced_to_jack() && _slave ) {
+	    follow_slave(nframes,0);
+	}
 	if (synced_to_jack() && waiting_to_start) {
 		if ( _engine.transport_state() == AudioEngine::TransportRolling) {
 			actually_start_transport ();
@@ -301,9 +304,11 @@
 		}
 		
 		if (!_exporting && _slave) {
+		    if( !synced_to_jack() ) {
 			if (!follow_slave (nframes, 0)) {
 				return;
 			}
+		    }
 		} 
 
 		if (_transport_speed == 0) {
@@ -733,9 +738,11 @@
 	}
 
 	if (!_exporting && _slave) {
+	    if( !synced_to_jack() ) {
 		if (!follow_slave (nframes, 0)) {
 			return;
 		}
+	    }
 	} 
 
 	if (_transport_speed == 0) {
diffs.diff (876 bytes)   

torbenh

2008-09-23 11:31

reporter   ~0005148

the problem is, that the start_transport() function only sets
waiting_to_start = TRUE, but does not do anything further.

waiting_to_start is evaluated in Session::process()
before calling (this->*process_function) (nframes);

this results in ardours internal transport starting one period too late.

the attached file diffs.diff fixes the problem.
there might be a solution which is more elegant, but i guess this
is up to paul :)

because i dont understand why actually_start_transport cant be called here.

paul

2008-10-11 11:59

administrator   ~0005180

torben's suggestion for a "clean" fix has been applied to 2.X and 3.0. it has not been thoroughly tested and may require changes to hydrogen since it was reported that they modified their code to workaround this bug in ardour. however, ardour's behaviour was clearly incorrect before and is now correct, so i am marking this resolved.

system

2020-04-19 20:12

developer   ~0021534

Issue has been closed automatically, by Trigger Close Plugin.
Feel free to re-open with additional information if you think the issue is not resolved.

Issue History

Date Modified Username Field Change
2007-06-24 10:11 lincoln New Issue
2007-07-17 22:45 torbenh3 Note Added: 0004150
2007-07-18 01:11 torbenh3 File Added: transport-diff.diff
2007-07-18 01:15 torbenh3 Note Added: 0004151
2007-07-18 03:19 torbenh3 Note Added: 0004153
2007-07-18 03:19 torbenh3 File Added: jack-transport-start-at-zero-fix.diff
2007-07-18 20:54 torbenh3 Note Added: 0004160
2007-08-15 12:44 lincoln Note Added: 0004268
2008-09-12 23:42 torbenh Note Added: 0005133
2008-09-13 08:00 lincoln Note Added: 0005135
2008-09-23 11:19 torbenh File Added: diffs.diff
2008-09-23 11:31 torbenh Note Added: 0005148
2008-10-11 11:59 paul cost => 0.00
2008-10-11 11:59 paul Status new => resolved
2008-10-11 11:59 paul Resolution open => fixed
2008-10-11 11:59 paul Assigned To => paul
2008-10-11 11:59 paul Note Added: 0005180
2020-04-19 20:12 system Note Added: 0021534
2020-04-19 20:12 system Status resolved => closed