View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0008001 | ardour | bugs | public | 2020-04-09 15:26 | 2020-06-08 15:26 |
Reporter | mpk | Assigned To | paul | ||
Priority | normal | Severity | minor | Reproducibility | always |
Status | closed | Resolution | fixed | ||
Product Version | 6.0-pre1 | ||||
Summary | 0008001: Playhead and audio get out of sync when loop range is changed whilst playing | ||||
Description | If 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 Reproduce | 1. 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 Information | Observed with both ALSA and JACK backends. | ||||
Tags | No tags attached. | ||||
|
Reproduced |
|
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. |
|
Fixed in 6.0-pre1-282-gfc34626e50 |
|
Sorry, but I cannot confirm it's fixed (see https://johannes-mueller.org/looptest.mp4 (Forgive my lousy guitar playing :))) |
|
Where is the bug in the video? There's no sound the playhead movement is correct. |
|
Oh my bad, it was just muted. Still I cannot reproduce this :( |
|
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. |
|
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 { |
|
request_locate() will cause a de-click and not a seamless loop. |
|
If we allow changing the loop range while looping, Ardour should not locate when the boundaries are changed, but continue playback. |
|
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. |
|
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. |
|
I cannot reproduce this error. Could you try to create small test session, archive it (Session > Archive) and upload it? |
|
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 |
|
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 |
|
What period size/count settings are you using? Which loop xfade options (Edit > Preferences > Transport > Looping) ? |
|
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 |
|
(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. |
|
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 |
|
This might be fixed soon. 3+ days of debugging work so far. |
|
Should now be fixed. Wow, that was hard! :) |
|
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. |
|
check you're at a9360eb6d6 or later, please. |
|
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. |
|
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. |
|
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. |
|
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. |
|
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 |
|
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. |
|
"problem" 2 was actually by design. Reading your comment makes it much more obvious that it's a stupid design. |
|
Yes, I noticed that problem 1 is not fixed. I agree that we should reinstate loop_changing. |
|
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. |
|
Confirmed, thanks. |
|
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 |
|
applied and pushed, thanks! |
|
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. |
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 | |
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 | |
2020-06-08 15:26 | anonymous | Note Added: 0024437 | |
2020-06-08 15:26 | anonymous | Status | resolved => closed |