View Issue Details

IDCategoryLast Update
0008001bugs2020-05-17 16:45
ReportermpkAssigned Topaul 
Reproducibilityalways 
Status resolvedResolutionfixed 
Product Version6.0-pre1 
Fixed in Version 
Summary0008001: Playhead and audio get out of sync when loop range is changed whilst playing
DescriptionIf the loop range is adjusted whilst playback is looping, Ardour does not update the audio loop points immediately so the visual playhead and the audio output get out of sync.
Steps To Reproduce1. Set loop range
2. Play loop range (L) and leave looping
3. Drag either loop marker, or change duration of loop range with keyboard shortcuts

Almost always, the display shows the playhead looping the new loop range, but the audio plays the old loop range. After a few repetitions (up to 10 seconds sometimes) the audio loop is updated to the new loop range, but the visual playhead is now out of sync with the audio.
Additional InformationObserved with both ALSA and JACK backends.
TagsNo tags attached.

Relationships

related to 0008111 resolvedpaul Ardour 6.0.rc1.298 Huge spike when looping audio tracks +40dB 

Activities

johmue-eo

2020-04-09 19:23

developer   ~0021272

Reproduced

johmue-eo

2020-04-09 21:23

developer   ~0021273

I tried to track this down a bit. It turns out that in session_process.cc:951 ::overwite_some_buffers() is not called when the loop range is changed. That's because the event is queued in session.cc:1556 with boost::shared_ptr<Track>().

If a call to ::overwrite_some_buffers () is added at session_process.cc:955 the elongated loop is not played to the end, but somewhere between the old loop end and the new loop end the audio starts from the loop start. This is probably due to some issue in buffer overwriting in DiskReader.

x42

2020-04-12 11:52

administrator   ~0021311

Fixed in 6.0-pre1-282-gfc34626e50

johmue-eo

2020-04-12 13:04

developer   ~0021312

Sorry, but I cannot confirm it's fixed (see https://johannes-mueller.org/looptest.mp4 (Forgive my lousy guitar playing :)))

x42

2020-04-12 19:31

administrator   ~0021316

Where is the bug in the video? There's no sound the playhead movement is correct.

x42

2020-04-12 19:33

administrator   ~0021317

Oh my bad, it was just muted. Still I cannot reproduce this :(

mpk

2020-04-13 08:44

reporter   ~0021327

Hi, thanks for working on this. Just to confirm the bug still exists for me too on 6.0.pre1.281-dbg. Let me know if there's anything I can do to help debugging this. I am able to build from git source.

mpk

2020-04-13 15:02

reporter   ~0021329

Just had another look. The current code works if the playhead is not within the new loop range, but the bug still exists for the other case. With the attached patch - i.e. ignoring the current playhead position and always relocating to the start of the new loop - things work.

Mostly... there is often a glitch when the playhead passes through the end of the old loop range - I think this is the old cross-fade to the loop start. Indeed this disappears when Loop Fades is set to No fades at loop boundaries.

loop.diff (1,329 bytes)
diff --git a/libs/ardour/session.cc b/libs/ardour/session.cc
index 900cd90372..f3e9f6e687 100644
--- a/libs/ardour/session.cc
+++ b/libs/ardour/session.cc
@@ -1533,27 +1533,14 @@ Session::auto_loop_changed (Location* location)
 
 		if (get_play_loop ()) {
 
-			if (_transport_sample < location->start() || _transport_sample > location->end()) {
-
-				/* new loop range excludes current transport
-				 * sample => relocate to beginning of loop and roll.
-				 */
-
-				/* Set this so that when/if we have to stop the
-				 * transport for a locate, we know that it is triggered
-				 * by loop-changing, and we do not cancel play loop
-				 */
-
-				loop_changing = true;
-				request_locate (location->start(), MustRoll);
-
-			} else {
+			/* Set this so that when/if we have to stop the
+			 * transport for a locate, we know that it is triggered
+			 * by loop-changing, and we do not cancel play loop
+			 */
+			loop_changing = true;
 
-				/* schedule a buffer overwrite to refill buffers with the new loop. */
-				SessionEvent *ev = new SessionEvent (SessionEvent::OverwriteAll, SessionEvent::Add, SessionEvent::Immediate, 0, 0, 0.0);
-				ev->overwrite = LoopChanged;
-				queue_event (ev);
-			}
+			/* Relocate to beginning of loop and roll. */
+			request_locate (location->start(), MustRoll);
 		}
 
 	} else {
loop.diff (1,329 bytes)

x42

2020-04-13 15:24

administrator   ~0021330

request_locate() will cause a de-click and not a seamless loop.

x42

2020-04-13 15:27

administrator   ~0021331

If we allow changing the loop range while looping, Ardour should not locate when the boundaries are changed, but continue playback.

mpk

2020-04-13 15:56

reporter   ~0021332

Yes, I agree - just pointing out that the bug happens in the "else" branch of this section. It seems the queued event is acted upon by the audio engine and the gui at different times.

krischan941

2020-04-19 17:20

reporter   ~0021403

Hello, I just want to confirm this bug exists for me (Ardour 6.0.rc1.2). A short video:
https://www.dropbox.com/s/4pk898jynh1rdph/Loop%20Bug.mp4?dl=0
When shorten the lenght of the loop while playing it, the first loop after shortening plays well. The second one goes wrong. This happens in most loop cases for me.

paul

2020-05-05 16:50

administrator   ~0024072

I cannot reproduce this error. Could you try to create small test session, archive it (Session > Archive) and upload it?

mpk

2020-05-05 18:31

reporter   ~0024073

Yes, it's not completely consistent, sometimes requiring a few goes to trigger it, and may take several repetitions of the new loop before the skip occurs. I get an error message "archiving failed" when trying to archive, so I've just compressed the session folder and uploaded.

There's also a video showing the bug - the audio glitch on the old loop boundary at 00:24, and then the skip when the playhead gets out of sync at 00:29.

https://drive.google.com/drive/folders/1XnI64y1e2i_t6qIuhDOeLfaIN1gqWXTk?usp=sharing

krischan941

2020-05-06 10:50

reporter   ~0024084

I can always trigger it when changing the loop size while the playhead always moves within the old and the new loop (during the change of the loop size).
With mpk's session:
https://www.dropbox.com/s/n5ps3t0ddzfccqj/LoopBug.mp4?dl=0
https://www.dropbox.com/s/gvnik8vismfrsxe/LoopBug2.mp4?dl=0

paul

2020-05-06 14:55

administrator   ~0024085

What period size/count settings are you using? Which loop xfade options (Edit > Preferences > Transport > Looping) ?

mpk

2020-05-06 15:09

reporter   ~0024086

The last example was recording with buffer size 512 samples, periods 3 (48 kHz). But I experience the bug at all sample rates and buffer sizes.

Loop xfade option is Cross-fade loop end and start. As I said in #c21329 t

mpk

2020-05-06 15:11

reporter   ~0024087

(sorry posted that too soon...)

Loop xfade option is Cross-fade loop end and start. As I said in #c21329, the glitch part of the bug doesn't happen if No fades is chosen, but the playhead sync error still occurs.

krischan941

2020-05-10 14:51

reporter   ~0024118

My period size with alsa or jack is 2 and my loop xfade option is cross-fade loop end and start.
Another thing which might be relate to this:
https://www.dropbox.com/s/ga1qjwebdpjwdnb/LoopBug3.mp4?dl=0
1. Play a loop with
2. after some loops press "L" or the "loop-button" again so that the playhead moves along
3. After some time it plays some audio from the loop

paul

2020-05-10 15:16

administrator   ~0024119

This might be fixed soon. 3+ days of debugging work so far.

paul

2020-05-12 19:09

administrator   ~0024126

Should now be fixed. Wow, that was hard! :)

johmue-eo

2020-05-12 19:37

developer   ~0024127

Last edited: 2020-05-12 19:38

View 2 revisions

Fix confirmed. No breakages seen up to now.

One minor observation: When clicking the loop button while transport is rolling in non loop, the loop does not get activated. The location does jump to the beginning of the loop, but the loop button remains inactive and the transport plays accordingly beyond the the end of the loop.

Ideally when the user activates looping the location should only jump to the beginning of the loop, if its outside the loop range. If inside, it should just play from there. But that's a nice to have.

paul

2020-05-12 20:20

administrator   ~0024128

check you're at a9360eb6d6 or later, please.

johmue-eo

2020-05-12 20:35

developer   ~0024129

I am, appearently: Git version: 6.0-rc1-283-g7a0427201c ; That's one after a9360eb6d6.

Just retried on 5120b650c5, master as of now. Same behavior. Need to go via Stop in order to change from Non Loop Play to Loop Play.

mpk

2020-05-13 08:00

reporter   ~0024130

Thanks @paul for your work on this. I've just built and tested this (Ardour6.0.rc1.235 (built using 6.0-rc1-285-g5120b650c5 and GCC version 10.1.1 20200507 (Red Hat 10.1.1-1))) and the out of sync bug seems to be resolved.

I still experience three issues involving looping though:

1. When extending the loop range while looping there is a persistent audio glitch on the OLD loop boundary after adjusting the loop. e.g. here the pipe characters are the loop boundaries, the angle brackets are the loop crossfades. It sounds like the crossfade from the old loop is retained at "x" in the new loop.

OLD LOOP: ----------------|<------------------------->|--------------------------
NEW LOOP: ----------------|<-------------------------x---------->|---------------

2. When shortening the loop range while looping AND the playhead (PH) is outside the new loop range, loop play is exited, ardour jumps to the start of the loop range and continues normal play from there.

OLD LOOP: ----------------|<-------------------PH---->|--------------------------
NEW LOOP: ----------------|PH------------->|-------------------------------------

3. The third is 0008017 which still occurs.

I also confirm @johmue-eo's issue in 0008001:0024129. Will try to look at the code later today.

mpk

2020-05-13 17:43

reporter   ~0024135

OK, regarding the second issue in my comment 0008001:0024130, the problem is that the new check in Session::non_realtime_stop from commit a9360eb6d6a4eb0c1b59c7f3a3f7be664c2c0bb8 doesn't work.

_transport_sample is compared to _locations->auto_loop_location()->start(), but in my testing the playhead has not yet moved to the start of the loop. Therefore unset_play_loop() is always called.

mpk

2020-05-13 17:54

reporter   ~0024136

I've just tried commenting out unset_play_loop() in Session::non_realtime_stop and this fixes issues 1 and 2 above and also @johmue-eo's issue switching from Play to Loop.

Of course the downside is that loop mode remains active and must be cancelled by pressing L before normal play, but perhaps this helps a bit.

It does not resolve 0008017.

mpk

2020-05-13 18:18

reporter  

0001-Reinstate-Session-loop_changing.patch (2,317 bytes)
From 28885801f2d1c01b9977f10928adc65d363c8e7d Mon Sep 17 00:00:00 2001
From: Mark Knoop <mark@markknoop.com>
Date: Wed, 13 May 2020 19:11:47 +0100
Subject: [PATCH] Reinstate Session::loop_changing

---
 libs/ardour/ardour/session.h     |  1 +
 libs/ardour/session.cc           |  2 ++
 libs/ardour/session_transport.cc | 12 ++----------
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/libs/ardour/ardour/session.h b/libs/ardour/ardour/session.h
index 2447b09f99..a54166937e 100644
--- a/libs/ardour/ardour/session.h
+++ b/libs/ardour/ardour/session.h
@@ -1452,6 +1452,7 @@ private:
 	 */
 	pframes_t               _pframes_since_last_mtc;
 	bool                     play_loop;
+	bool                     loop_changing;
 	samplepos_t              last_loopend;
 
 	boost::scoped_ptr<SessionDirectory> _session_dir;
diff --git a/libs/ardour/session.cc b/libs/ardour/session.cc
index 2b530ba907..3d914dd263 100644
--- a/libs/ardour/session.cc
+++ b/libs/ardour/session.cc
@@ -223,6 +223,7 @@ Session::Session (AudioEngine &eng,
 	, _send_qf_mtc (false)
 	, _pframes_since_last_mtc (0)
 	, play_loop (false)
+  , loop_changing (false)
 	, last_loopend (0)
 	, _session_dir (new SessionDirectory (fullpath))
 	, _current_snapshot_name (snapshot_name)
@@ -1516,6 +1517,7 @@ Session::auto_loop_changed (Location* location)
 				 * by loop-changing, and we do not cancel play loop
 				 */
 
+				loop_changing = true;
 				request_locate (location->start(), MustRoll);
 
 			} else {
diff --git a/libs/ardour/session_transport.cc b/libs/ardour/session_transport.cc
index 37dcd91457..2f3c2e44f3 100644
--- a/libs/ardour/session_transport.cc
+++ b/libs/ardour/session_transport.cc
@@ -1474,16 +1474,8 @@ Session::non_realtime_stop (bool abort, int on_entry, bool& finished)
 
 	if (ptw & (PostTransportClearSubstate|PostTransportStop)) {
 		unset_play_range ();
-		if (!Config->get_loop_is_mode()) {
-			if (get_play_loop()) {
-				/* do not unset loop playback if we've just
-				   located back to the start of the loop (i.e. to
-				   prepare to play the loop.
-				*/
-				if (_transport_sample != _locations->auto_loop_location()->start()) {
-					unset_play_loop ();
-				}
-			}
+		if (!Config->get_loop_is_mode() && get_play_loop() && !loop_changing) {
+			unset_play_loop ();
 		}
 	}
 
-- 
2.26.2

mpk

2020-05-13 18:18

reporter   ~0024137

Attached patch reinstates Session::loop_changing just for this case and fixes problem 2 and the need to go via stop in order to switch into loop play.

Actually problem 1 (the glitching) is not fixed by this.

paul

2020-05-13 18:25

administrator   ~0024138

"problem" 2 was actually by design. Reading your comment makes it much more obvious that it's a stupid design.

paul

2020-05-13 18:31

administrator   ~0024139

Yes, I noticed that problem 1 is not fixed.

I agree that we should reinstate loop_changing.

paul

2020-05-14 00:54

administrator   ~0024141

OK, glitch should be fixed now, and I manually merged the loop_changing re-instatement patch.

This still leaves some transport-related issues and 0008017.

mpk

2020-05-14 07:26

reporter   ~0024143

Confirmed, thanks.

mpk

2020-05-17 15:20

reporter   ~0024180

There are still some problems with loop play which I'm pretty sure are fixed by the attached patch.

0001-Fix-repeated-toggling-of-loop-mode.patch (1,918 bytes)
From f1cf125a1d1227cf5af12bb92d34ca85c64a4ffd Mon Sep 17 00:00:00 2001
From: Mark Knoop <mark@markknoop.com>
Date: Sun, 17 May 2020 16:08:49 +0100
Subject: [PATCH] Fix repeated toggling of loop mode

Calling Session::set_play_loop repeatedly (e.g. LLL) should toggle in and out
of loop play. Previously transport needed to be stopped before loop play could
be started for a second or subsequent time. This uses the loop_changing boolean
to flag that Session::non_realtime_stop should not unset the loop.

Also, Session::non_realtime_stop must reset loop_changing to false after use so
it does not affect the next transport action.
---
 libs/ardour/session_transport.cc | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/libs/ardour/session_transport.cc b/libs/ardour/session_transport.cc
index a5e8f3ea7f..92ab447179 100644
--- a/libs/ardour/session_transport.cc
+++ b/libs/ardour/session_transport.cc
@@ -1483,6 +1483,9 @@ Session::non_realtime_stop (bool abort, int on_entry, bool& finished)
 		}
 	}
 
+	/* reset loop_changing so it does not affect next transport action */
+	loop_changing = false;
+
 	if (!_transport_fsm->declicking_for_locate()) {
 
 		DEBUG_TRACE (DEBUG::Transport, X_("Butler PTW: locate\n"));
@@ -1593,7 +1596,11 @@ Session::set_play_loop (bool yn, bool change_transport_state)
 		merge_event (new SessionEvent (SessionEvent::AutoLoop, SessionEvent::Replace, loc->end(), loc->start(), 0.0f));
 
 		if (!Config->get_loop_is_mode()) {
-			/* args: positition, disposition, flush=true, for_loop_end=false, force=true */
+			if (transport_rolling()) {
+				/* set loop_changing to ensure that non_realtime_stop does not unset_play_loop */
+				loop_changing = true;
+			}
+			/* args: position, disposition, flush=true, for_loop_end=false, force=true */
 			TFSM_LOCATE (loc->start(), MustRoll, true, false, true);
 		} else {
 			if (!transport_rolling()) {
-- 
2.26.2

paul

2020-05-17 16:45

administrator   ~0024182

applied and pushed, thanks!

Issue History

Date Modified Username Field Change
2020-04-09 15:26 mpk New Issue
2020-04-09 19:23 johmue-eo Assigned To => johmue-eo
2020-04-09 19:23 johmue-eo Status new => confirmed
2020-04-09 19:23 johmue-eo Note Added: 0021272
2020-04-09 21:14 johmue-eo Assigned To johmue-eo =>
2020-04-09 21:23 johmue-eo Note Added: 0021273
2020-04-12 04:44 x42 Assigned To => x42
2020-04-12 11:52 x42 Status confirmed => resolved
2020-04-12 11:52 x42 Resolution open => fixed
2020-04-12 11:52 x42 Note Added: 0021311
2020-04-12 13:04 johmue-eo Status resolved => confirmed
2020-04-12 13:04 johmue-eo Note Added: 0021312
2020-04-12 19:31 x42 Note Added: 0021316
2020-04-12 19:33 x42 Note Added: 0021317
2020-04-12 19:33 x42 Assigned To x42 =>
2020-04-12 19:33 x42 Assigned To => x42
2020-04-12 19:33 x42 Status confirmed => new
2020-04-12 19:33 x42 Assigned To x42 =>
2020-04-13 08:44 mpk Note Added: 0021327
2020-04-13 15:02 mpk File Added: loop.diff
2020-04-13 15:02 mpk Note Added: 0021329
2020-04-13 15:24 x42 Note Added: 0021330
2020-04-13 15:27 x42 Note Added: 0021331
2020-04-13 15:56 mpk Note Added: 0021332
2020-04-19 17:20 krischan941 Note Added: 0021403
2020-04-19 17:34 paul Assigned To => paul
2020-05-05 16:50 paul Note Added: 0024072
2020-05-05 18:31 mpk Note Added: 0024073
2020-05-06 10:50 krischan941 Note Added: 0024084
2020-05-06 14:55 paul Note Added: 0024085
2020-05-06 15:09 mpk Note Added: 0024086
2020-05-06 15:11 mpk Note Added: 0024087
2020-05-10 14:51 krischan941 Note Added: 0024118
2020-05-10 15:16 paul Note Added: 0024119
2020-05-12 19:09 paul Note Added: 0024126
2020-05-12 19:37 johmue-eo Note Added: 0024127
2020-05-12 19:38 johmue-eo Note Edited: 0024127 View Revisions
2020-05-12 20:20 paul Note Added: 0024128
2020-05-12 20:35 johmue-eo Note Added: 0024129
2020-05-13 08:00 mpk Note Added: 0024130
2020-05-13 17:43 mpk Note Added: 0024135
2020-05-13 17:54 mpk Note Added: 0024136
2020-05-13 18:18 mpk File Added: 0001-Reinstate-Session-loop_changing.patch
2020-05-13 18:18 mpk Note Added: 0024137
2020-05-13 18:25 paul Note Added: 0024138
2020-05-13 18:31 paul Note Added: 0024139
2020-05-14 00:54 paul Note Added: 0024141
2020-05-14 07:26 mpk Note Added: 0024143
2020-05-14 12:12 x42 Relationship added related to 0008111
2020-05-16 15:33 paul Status new => resolved
2020-05-17 15:20 mpk File Added: 0001-Fix-repeated-toggling-of-loop-mode.patch
2020-05-17 15:20 mpk Note Added: 0024180
2020-05-17 16:45 paul Note Added: 0024182