Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

softcut: routines for copying and grabbing content from buffer regions #1161

Merged
merged 12 commits into from Jul 27, 2020

Conversation

@csboling
Copy link
Contributor

@csboling csboling commented Jul 12, 2020

Refs #1151

  • Implements a BufDiskWorker routine for copying content from one point in a softcut buffer to another, with optional fade time and an argument to reverse the content while copying. As with buffer_clear_region, plumbing includes mono and stereo variants.
  • Implements a "render" routine for capturing the contents of an arbitrary portion of a softcut buffer, sampled at a caller-specified number of points, using the procedure described in this post. If the total number of frames in the requested region is less than the requested number of samples, frames are returned unaltered. Otherwise a simple peak detection scheme is used, skipping frames if the requested region is sufficiently large.

For debugging and demonstration I wrote this simple waveform visualizer script, presented with apologies for possible line ending issues and definite script-level bugs.

An assorted mash of remarks / questions / insecurities:

  • Nomenclature and method signatures - not sure about the choice of OSC paths in particular.
  • I think the algorithm(s) are right? I can hear fades working and stuff looks reasonable in the visualizer. Not sure how best to more thoroughly verify this kind of thing, I guess probably I should export WAVs and pull them up in Audacity side-by-side or something. Most likely needs some more thorough testing for edge cases.
  • Peak-finding downsampling is probably too aggressive, needs tuning for accuracy / perf tradeoff. I just cooked up a value that seemed to work OK to avoid xruns for large regions.
}

softCutClient->renderSamples(ch, argv[1]->f, argv[2]->f, sampleCt,
[=](float secPerSample, float start, size_t count, float* samples) {

This comment has been minimized.

@csboling

csboling Jul 12, 2020
Author Contributor

Here and here I demonstrate that I only pretend to know how to do Modern C++ things. All the other callbacks use regular function pointers and I understand that std::function is more costly, but I could not get this to compile with a function pointer when this lambda needs to value-capture the ch index. Other places in this file use addServerMethod with some capturing going on (I think?) but the compiler is not happy about this -- maybe because this is a lambda defined inside another lambda?

This comment has been minimized.

@catfact

catfact Jul 14, 2020
Collaborator

i think the overhead of the lambda is totally fine here. i don't think there are any cases of capturing elsewhere - there is just a lot of static storage.

@csboling csboling requested a review from catfact Jul 12, 2020
@tehn
Copy link
Member

@tehn tehn commented Jul 12, 2020

holy smokes!! this is huge--- looking forward to digging in--- i was just thinking the other day about buffer ops, and waveforms have been in the back of my head since the projects' inception...

Copy link
Collaborator

@catfact catfact left a comment

looks great - thanks for taking this on.

i haven't got around to running on norns yet... if it works for others i'm good to merge

x = 1.f;
phi = 0.f;
}

This comment has been minimized.

@catfact

catfact Jul 14, 2020
Collaborator

equal-power crossfade also an option, using x as phase:

        static float raisedCosFadeIn(float unitphase) {
            return 0.5f * (cosf(M_PI * (1.f + unitphase)) + 1.f);
        };

        static float raisedCosFadeOut(float unitphase) {
            return 0.5f * (cosf(M_PI * unitphase) + 1.f);
        };
}

softCutClient->renderSamples(ch, argv[1]->f, argv[2]->f, sampleCt,
[=](float secPerSample, float start, size_t count, float* samples) {

This comment has been minimized.

@catfact

catfact Jul 14, 2020
Collaborator

i think the overhead of the lambda is totally fine here. i don't think there are any cases of capturing elsewhere - there is just a lot of static storage.

@csboling
Copy link
Contributor Author

@csboling csboling commented Jul 15, 2020

  • Changed fade curve to raised-cosine.
  • Realized I was only fading in the beginning of the copied content and not fading out its end 😬
  • For doubling an existing softcut loop exactly I realized that you probably want to just mix the existing content in the buffer with the copied content and not do an additional crossfade since the loop content will already have its endpoints faded -- you just want to sum the faded ends. Therefore added a "preserve" option to keep / attenuate the existing content you overwrite with the copy, rather than eliminating it entirely, which should allow you to use something like (I think)
    softcut.buffer_copy_stereo(0,             -- start of copy source
                               length,        -- start of copy destination
                               length + 0.1,  -- source length including post-roll, so s/0.1/fade_time/
                               0.0,           -- no crossfading during copy
                               1.0)           -- mix existing material 1-to-1 with copied material
    I tried editing Samsara along these lines and it seems anecdotally to work, but I am still getting a pop occasionally when doubling the buffer while recording. Need some more time digging to know why but I thought I would post these updates in case someone else gets a chance to try this patch.
  • For my own application I think I also want to add a fade time option to clearBuffer to allow emptying a buffer region without introducing discontinuities. This would involve adding an argument to the OSC methods, not sure if that kind of thing constitutes a problematic API change. May be best to make a follow-up PR for this.
}

float BufDiskWorker::mixFade(float x, float y, float a, float b) {
return a * x + b * y;

This comment has been minimized.

@csboling

csboling Jul 15, 2020
Author Contributor

Split this out to a function so the mixing approach could be tweaked. Not sure if this should use the same mixing calculation as ReadWriteHead::mixFade, I don't think I understand how/why this works so I left it linear here. Also maybe need to think about clipping behavior?

This comment has been minimized.

@catfact

catfact Jul 18, 2020
Collaborator

i actually think it's fine to exceed unity range in the buffer:

  • for live output, the only thing that matters is the level at the final sink, and there are many gain stages left to go before the buffer contents make it to the DAC. if anything we should actually add a hardclip to the final ouput of SoftcutClient, but i'm also jappy to let JACK/drivers handle it.
  • for saving the buffer to disk, we let libsndfile perform clipping at the same time as converting to the requested output sample format.

crossfade options are a whole huge topic that i'd love to discuss...

  • for the (generally good[*1]) assumption that real-world signals are uncorrelated, we want the sum of each signal's power (squared amplitude) to remain constant.
  • $sin^2 + cos^2 = 1$ is a convenient relation for building curves with that property.
  • in crone/softcut we've used both "raised cosine" (for a gradual fade) and "half cosine" for a fast fade. [*2]
  • tbh, neither is really ideal and don't create "perceptual linearity." [*3] the 2.0 revision has many new facilities for efficient table-based envelopes, and the final touch to that would be making some perceptually linear in/out tables for this purpose.

anyways maybe we can raise a TODO issue for updating the crossfades perfectly; it's definitely not a big deal at this point to leave them linear or whatever.

[*1] when signals are perfectly correlated, you want "equal voltage" instead of "equal power." it's prohibitive to be determining this in real time for all mix points of course; but you could actually do it for an offline operation.
[*2] (...but i should make sure that the phase arguments are correct in all cases; raised-cosine still sometimes being summed incorrectly...)
[*3] it's even weirder in context of softcut xfades, because overdub and record levels are also crossfading, so correlation with signals from the past comes into play.

@tehn
Copy link
Member

@tehn tehn commented Jul 15, 2020

For my own application I think I also want to add a fade time option to clearBuffer to allow emptying a buffer region without introducing discontinuities. This would involve adding an argument to the OSC methods, not sure if that kind of thing constitutes a problematic API change. May be best to make a follow-up PR for this.

this could be pulled off without a breaking change on the lua API side because a default arg could be assumed. i think we can change the OSC layer presently without treating it like an API layer--- this of course could change in the future if there was an alternative front-end to crone (instead of matron) but i don't think that's going to happen--- if anything there's been talk of changing the IPC from OSC to something else, though that's not super high priority.

but yes, clear with fades with be a great feature. i'm very enthusiastic about these buffer ops! hoping to test this week, thank you for your efforts!

also yeah--- probably makes sense for a separate PR :)

@tehn
tehn approved these changes Jul 18, 2020
@catfact
Copy link
Collaborator

@catfact catfact commented Jul 18, 2020

fade time option to clearBuffer

re: OSC/lua API for this, i agree:

  • lua API can use extra arg with default value, non-breaking
  • in OSC layer, i haven't figured out a way to use optional arguments. clearBuffer could take a new arg (breaking) or make a new clearBufferWithFade (non-breaking.) AFAIK i'm the only one using crone without matron, so either way is fine. the former is more consistent with the style of existing commands, the latter probably would have been smarter in the first place. (oh well.)
Sam Boling added 2 commits Jul 18, 2020
@tehn
Copy link
Member

@tehn tehn commented Jul 22, 2020

excellent.

i'll do more testing tonight with live input, and attempt some high-stress (queue up a ton of buf workers) but i suspect this is probably good to go. there will be caveats as to how this functionality can be used, but i think that's reasonable.

@tehn
Copy link
Member

@tehn tehn commented Jul 25, 2020

ok, i'm feeling good about this and feel it can be merged to main. that way we might get a few additional testers prior to a release. @csboling @catfact any concerns?

@csboling
Copy link
Contributor Author

@csboling csboling commented Jul 25, 2020

Testing last night I finally got it through my head that you get weird behavior if the source region and target region overlap, so it needs to use the memmove trick (changing which direction the copy loop goes depending on the relative positions of the source / target). Going to work on this this morning and then I think it's good to go.

@tehn
Copy link
Member

@tehn tehn commented Jul 25, 2020

good catch. i'll try to analyze similar edge cases

@csboling
Copy link
Contributor Author

@csboling csboling commented Jul 25, 2020

  • Fixed handling of overlapping regions in the non-reversed copy case. This allows for loop-doubling without discontinuities, I tried this out with Samsara and it (finally) works as expected 🔁🔁
  • When reversing content, there's no way to avoid garbling the overlap without allocating additional storage the size of the overlap and doing another copy (i.e. potentially the size of the whole softcut buffer(s)) so this is left unhandled. Added a remark in the Luadoc that this is not handled when reverse is set.
  • Use the complementary-sines mixing approach for crossfades for consistency with ReadWriteHead. Avoid the trig overhead when not crossfading and just use a linear mix since it's mathematically equivalent.
@tehn tehn merged commit 41a99e7 into monome:main Jul 27, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants