repeat: after last track sample, fill buffer with samples from track start

[ronso0]

or with samples from end if playing in reverse.

This enables gapless playback when repeat is enabled.

Previously, the last buffer at track end was padded with silence which makes the track longer than it actually is. With loop tracks (length is exactly n beats) this causes beat offsets with BPM-sync'ed tracks (non-quantized).
Quantize would fix the offset but the audio gap remains AFAICT.

See #9842 and https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/repeat.20accuracy

NOTE This is just a minimal invasive POC hack that abuses LoopingControl::nextTrigger because

• that is called anyways
• can already read track samples and
• does basically the same wrapping for loops

Maybe there is a better place or a separate function is required.
Also some cleanup needs to be done in EngineBuffer::processTrackLocked, there are residues.

Probably related: #11381

Closes #9842

[ronso0]

@mevsme please give this a shot with your test tracks.
Once the CI passed you can download build artifacts, see https://github.com/mixxxdj/mixxx/wiki/Testing#github-pull-requests

[ronso0]

The ReverseTest failed, though only on Windows. In a previous run (before I pushed a comment update) all tests on all OS passed.

Warning [Engine]: Buffer does not match "ReverseTest" , actual buffer written to  "reference_buffers/ReverseTest.actual"
D:\a\mixxx\mixxx\src\test/signalpathtest.h(196): error: Value of: false
  Actual: false
Expected: true

[ronso0]

I could reproduce the issue and also confirm the fix 🎉
100 BPM loop I used is here https://free-loops.com/8603-cut-beat-100.html

Steps tp reproduce:

• select the biggest audio buffer (85 ms) to increase buffer size / track end overlap
• load loop to deck1
• into deck2 load another track with a beat grid (const beats) that is long enough to allow deck1 to repeat like 10 times before deck2 finishes, either with 100 BPM or sync'ed to deck1, doesn't matter
• enable quantize on both decks to make sure they are in sync
• start loop, start track2
• quickly disable quantize on both decks before loop track is repeated
• watch the beatgrid alignment

[ronso0]

Unfortunately this fix breaks quantization for repeating tracks. (which is at least consistent with beatloops != n beats, as in 2.3)
Looking into it.

[ronso0]

Unfortunately this fix breaks quantization for repeating tracks. (which is at least consistent with beatloops != n beats, as in 2.3) Looking into it.

I fixed this.

Though I wonder whether quantization for repeat is a) actually desired / expected, or if b) this was just an implicit side effect of the previous repeat implementation.

        if (repeat_enabled) {
            double fractionalPos = at_start ? 1.0 : 0;
            doSeekFractional(fractionalPos, SEEK_STANDARD);   <--- triggers phase sync
        } else {

I suspect it's b), at least the quantize description doesn't mention it (though it doesn't mention other aspects either, e.g. phase sync of quantized decks).

Anyway, now the behaviour is exactly as before + the audio gap fix.

It would be nice to also address the TODO (that also applies to the previous implementation, and when scratching is stopped and tracks are resync'ed): currently there is one unsyced buffer played before tracks are quantized.
https://github.com/mixxxdj/mixxx/pull/11532/files#diff-c960dda3b73ecd4b1b7801a42ddf7c5b9f6427af1e83c23368dffbddb06ca391R1015-R1020

    if (m_pRepeat->toBool() && m_pQuantize->toBool() &&
            (m_filepos_play > playpos_old) == backwards) {
        // TODO() The resulting seek is processed in the following callback
        // That is to late
        requestSyncPhase();
    }

[daschuer]

requestSyncPhase() requires an other track playing to be in sync. Did you consider a solution where the repeat + quantize becomes a beat loop that the track stays in sync with itself?

[ronso0]

yes, this is only supposed to sync phase when there's another track. I'm still not sure if that's a good idea, and for which use case it helps, considering users expecting beat and phase sync would set the loop manually, i.e. don't rely on phase sync to snap to the correct beat.

and no, this should not be an extension of the original beat loop fix.

[daschuer]

This looks already like a good improvement. Is it a lat minute 2.3.5 candidate?

[daschuer]

I have created a 1 s 440 Hz wav track and play it in repeat from the lead in.

In audacity it has full power from start to end.
Quantize is disabled:

This PR:

plain 2.3

In this PR the track starts with click sound (no ramping in).
There is a ramping out, but no proper cross fading like when using normal loops.

The original code shows a gap in the first iteration. Is this a caching issue? Later it kind of works, but we see ramping in and out.

What is the desired solution?
Use ramping and crossfading? This will make the end and the beginning overlap.
Don't use ramping and rely on the recording?
I think I would vote for the later, because we have also no ramping when starting from the lead in.

We also have the issue that we cannot play life recordings gap-less. This would require a start to end transition without overlapping and fading.

This is different if we consider Quantize. I think we can expect that with quantize enabled the track plays beat matches with itself. In this case crass-fading is desired.

Mmm, I am afraid we need to go back to the cutting table.

I like the idea of using the looping code. It has already the cross fade feature. If we use the silence after the track and before the track for cross-fading, it will have no addible effect and the back-to-back mode without cross fading works implicit.

[ronso0]

This could be for 2.3.5 but it has no prio IMO, given the issue seems to exist very long already, it's only one older report and not much interest otherwise.

[ronso0]

In this PR the track starts with click sound (no ramping in).
There is a ramping out, but no proper cross fading like when using normal loops.

The sine wave file has no ramping and no silence itself, right? If yes, could try with a loop from track start and track end, both 2.3 and this PR, and post a screenshot, too?

I have a local experimental commit that does the quatization (for repeat) already in LoopingControl and it unveiled an play pos logging issue in ReadAheadManager that would somehow revert the applied quantization. So yes, even though this is an improvement there's more to be done to have proper gapless playback.
I'll share that WIP stuff in another commit so this PR could be merged.

[ronso0]

Here's my comparison of 2.3 / PR with a 2s 440 Hz sine wave
@48 kHz, 85 ms buffer, edit RubberBand, all tests played from track start (no preroll part).
top to bottom (like the track labels):
2.3 repeat
2.3 full-track loop
PR repeat
PR full-track loop

I have no explanation for the alternating fading time, though I didn't yet look into the reader at all where the cross-fading seems to be done. Maybe it's beause of the mismatch buffer size vs. available samples (original silence issue). If so, with a sine wave with exactly [n * buffer length] we should see identical crossfades.
FWIW I see (recording waveform) that only with 85 ms buffer -- with 5 ms all fades are ~5 ms.

I'd say this is good to go because at least the repeat behaviour is now consistent with the (full-track) loop behaviour.

[daschuer]

I can reproduce the same behavior with a shorter loop starting at the beginning.
When I start half a buffer later, I can see the ramping in. So it is kind of working.

My assumption form a above is wrong that looping already allows gapless playback, because it uses for crossfading the last samples in a loop and the same amount of samples in front of the loop.
If this is in the lead-in, we may consider to skip the fade out, to have no notable loudness reduction at the end of the track.

Unfortunately the manual set out cue is randomly shifted after the track:

This is due to the quantitation effect of the engine buffer. It plays a bit beyond the track to be able to fill a very last buffer with the remaining samples form the track. The rest is padded with silence. The loop out is here placed at the end of the silence at a place that may not reached if less padding is required on the next loop iteration.

This seems to be a slightly related bug that is also happening in 2.4.

The window for last minute 2.3.5 merges is closed now and we are waiting for smoke test:
https://github.com/mixxxdj/mixxx/actions/runs/4906831351
This means we have no rush here and can merge this after all aspects are done.
I will look into this in more details one I have found time.

[daschuer]

ronso0#42 fixes the issue with cross-fading.
I cam now able to play my 1 s 440 Hz track without clicking sound forward and backwards.

[ronso0]

Thank you! I picked those commits and removed the debug output.

[ronso0]

With quantize enabled, a (visual) issue remains: when a track wraps around to the start, the visual position is in the preroll briefly before catching up to a qunatized position, though the recording (sine wave) shows continuous playback 🤷‍♂️

[daschuer]

How can this be reproduced? Does it also affect the track time display?

[ronso0]

I played the testing sine wave (2s) and a longer track. Both with repeat and quantize enabled.
When the short track jumps from end to start I can see it in the preroll for a glimpse. Not sure about the position label, will test later on.

[ronso0]

Seems to be a waveform issue only (on both decks btw), the palypos label is always in the positive range (I don't see a - when the playhead is in the preroll).

[daschuer]

This is a whole 440 Hz Track repeat looks reliable like that:

This is correct, because it x-fades from the last samples to the pre-roll.
Which is OK for this state of the PR.
If we desire gap-less playback we need to special case this without cross-fading.
out of scope of this PR.

[daschuer]

LGTM

[daschuer]

Oh I noticed this code was there, but got lost:
Here it is again:
#11704