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 buf worker #958

Closed
wants to merge 31 commits into from
Closed

Softcut buf worker #958

wants to merge 31 commits into from

Conversation

@catfact
Copy link
Collaborator

@catfact catfact commented Dec 1, 2019

this is WIP, but putting it up for early testing in case that's useful.

buffer management is now done with a dedicated worker thread; IPC interface makes requests that are placed on a job queue.

i've only implemented the softcut/buffer/read_mono command, will finish the rest and refactor/rewrite for disk efficiency.

i'd like as much testing coverage as possible for this since there are lots of edge cases:

  • requesting source channel not present in file
  • requesting position/durations that are OOB for source/dest
    etc
@catfact
Copy link
Collaborator Author

@catfact catfact commented Dec 1, 2019

btw, i'm gonna have probably quite a few crone PRs coming up that are dependent on each other and possiby with messy history. it might make sense to merge to a feature branch instead of master... lmk

emb added 2 commits Dec 1, 2019
emb
emb
@tehn
Copy link
Member

@tehn tehn commented Dec 1, 2019

i can help test these edge cases.

yes if you'd like to do a crone feature branch with several PR's i'm cool with that (but also fine with master if you prefer)

@catfact
Copy link
Collaborator Author

@catfact catfact commented Dec 1, 2019

ok, so far it seems to have the desired effect.

testing:

  • take any softcut-only script (like Sam or the demos)
  • modify it to have some periodic modulation of crone/softcut parameters. for example, this voice 1 tremolo:
  level_timer = metro.init()
  level_timer.time = 1/10
  level = 1
  level_timer.event = function()
     if level == 1 then level = 0 else level = 1 end
     softcut.level(1,level)
  end
  level_timer:start()

(the point being to generate some timing-sensitive OSC traffic between matron and crone)

  • perform some soundfile loading in the REPL, e.g:
  _norns.cut_buffer_read_mono("/home/we/dust/audio/hermit_leaves.wav", 10, 0, 30, 0, 1)

before this change, the tremolo would stop while the soundfile loading tied up crone's OSC handler. after the change it seems fine.

so i'll go ahead and finish the other buffer job types

@catfact
Copy link
Collaborator Author

@catfact catfact commented Dec 1, 2019

ok, added implementation and glue for remaining methods. (clear, read_stereo, write_mono, write_stereo)

haven't tested these yet.. clocking out for the moment. (and there are still disk optimizations to do; we are still seeking on every I/O frame which is idiotic but simple.)

there are quite a few parameters and cases (src/dst position and channels..) so it's not impossible that i typo'd something, switched param order somewhere &c.

(to be clear, all changes are on crone side - in SoftcutClient.cpp, BufDiskWorker.cpp - lua API is unchanged.)

@tehn
Copy link
Member

@tehn tehn commented Dec 8, 2019

@catfact do you expect to add much more to this branch? was going to start testing everything thoroughly

@catfact
Copy link
Collaborator Author

@catfact catfact commented Dec 11, 2019

y'know, maybe not really.

two things:

  • i started adding a callback via OSC when buffer I/O jobs are completed. but i realized that i didn't have a clear story about how these would be handled or what purpose they would serve, so i didn't have a great plan as to what data the callbacks should get (job type as string, buffer path as string, buffer index? timestamp of original work request?)

  • it would be more efficient to read/write blocks instead of single frames. but this is a bit more complex than it appears, particularly for reading. the nice thing about the single-frame I/O is that it handles arbitrary channel counts in the source file quite easily. otherwise we need to maintain arbitrarily large buffers for de-interleaving.

so i'm going to remove "do not merge" label. it is still pending test - i really have not tried all combinations of stereo/mono input/output and have basically just tested the mono case in both directions. but i think further improvements can be dealt with as separate issues.

@tehn
Copy link
Member

@tehn tehn commented Dec 19, 2019

not compiling presently on-device:

[46/49] Compiling crone/src/OscInterface.cpp
In file included from ../crone/src/BufDiskWorker.cpp:10:0:
../crone/src/OscInterface.h:73:60: error: ‘struct crone::BufDiskWorker::Job’ is private within this context
         static void notifyBufferJobComplete(BufDiskWorker::Job job);
                                                            ^~~
In file included from ../crone/src/BufDiskWorker.cpp:9:0:
../crone/src/BufDiskWorker.h:33:16: note: declared private here
         struct Job {
                ^~~
../crone/src/BufDiskWorker.cpp: In static member function ‘static void crone::BufDiskWorker::workLoop()’:
../crone/src/BufDiskWorker.cpp:83:50: warning: left operand of comma operator has no effect [-Wunused-value]
                     clearBuffer(bufs[job.bufIdx[0], job.startDst, job.dur]);
                                      ~~~~~~~~~~~~^
../crone/src/BufDiskWorker.cpp:83:57: warning: right operand of comma operator has no effect [-Wunused-value]
                     clearBuffer(bufs[job.bufIdx[0], job.startDst, job.dur]);
                                                     ~~~~^~~~~~~~

In file included from ../crone/src/main.cpp:12:0:
../crone/src/OscInterface.h:73:60: error: ‘struct crone::BufDiskWorker::Job’ is private within this context
         static void notifyBufferJobComplete(BufDiskWorker::Job job);
                                                            ^~~
In file included from ../crone/src/SoftCutClient.h:10:0,
                 from ../crone/src/main.cpp:11:
../crone/src/BufDiskWorker.h:33:16: note: declared private here
         struct Job {
                ^~~

In file included from ../crone/src/OscInterface.cpp:17:0:
../crone/src/OscInterface.h:73:60: error: ‘struct crone::BufDiskWorker::Job’ is private within this context
         static void notifyBufferJobComplete(BufDiskWorker::Job job);
                                                            ^~~
In file included from ../crone/src/OscInterface.cpp:15:0:
../crone/src/BufDiskWorker.h:33:16: note: declared private here
         struct Job {
                ^~~
../crone/src/OscInterface.cpp: In static member function ‘static void crone::OscInterface::notifyBufferJobComplete(crone::BufDiskWorker::Job)’:
../crone/src/OscInterface.cpp:790:59: error: ‘struct crone::BufDiskWorker::Job’ is private within this context
 void OscInterface::notifyBufferJobComplete(BufDiskWorker::Job job) {
                                                           ^~~
In file included from ../crone/src/OscInterface.cpp:15:0:
../crone/src/BufDiskWorker.h:33:16: note: declared private here
         struct Job {
                ^~~

side note:

the CI setup just generates docs, so the "all checks passed" is a little misleading

@catfact
Copy link
Collaborator Author

@catfact catfact commented Dec 19, 2019

oh sorry, i'll fix.

meantime try rolling back to e6b4fd8

@tehn
Copy link
Member

@tehn tehn commented Dec 19, 2019

no problem-- i also rolled back my earlier push and PR'd your repo for the same buffer fix

i'll do some testing on this!

@catfact
Copy link
Collaborator Author

@catfact catfact commented Dec 19, 2019

fixed compilation on x86 but not yet tried on pi. (should be fine.)

the broken part was when i pushed WIP for job-complete notifications. that's still in there and i'd welcome any thoughts on it. should now send /buffer_job_complete to matron with two arguments: name of job type (clear, read_mono, &c) and soundfile path (if applicable.)

this has clear use cases for debugging but i don't know whether its useful beyond that. buffer work queue is quite large, so there should be no need for queuing/throttling work requests on matron side, unless making hundreds or thousands of requests (which would be very weird design.) but maybe there are times when some action needs to wait on buffer job completion (e.g., save soundfile, then do something with it on disk.)

@tehn
Copy link
Member

@tehn tehn commented Dec 19, 2019

just checked, compiles on hardware.

re: buffer work completion callbacks--- i can see them being useful as you say. i haven't given much thought how to deal with the lua syntax.

i'll move on to checking functionality

@tehn
Copy link
Member

@tehn tehn commented Dec 19, 2019

oh, want to merge this? catfact#4

fix reads
@tehn
Copy link
Member

@tehn tehn commented Dec 20, 2019

yikes--- crone now segfaults when matron connects. (i'm manually start/stopping matron and crone)

i can continue looking at this on saturday, apologies for the delay

@catfact
Copy link
Collaborator Author

@catfact catfact commented Dec 20, 2019

Hm... Up til

@catfact catfact closed this Dec 20, 2019
@catfact
Copy link
Collaborator Author

@catfact catfact commented Dec 20, 2019

Oops

Up til e6b4fd8 was tested extensively on device...

@catfact catfact reopened this Dec 20, 2019
@catfact
Copy link
Collaborator Author

@catfact catfact commented Dec 20, 2019

alright, this is too borked now with the merges and errors. closing and will re-open when i can sit down with it again

@catfact catfact closed this Dec 20, 2019
@catfact catfact mentioned this pull request Dec 20, 2019
@catfact catfact deleted the catfact:softcut-buf-worker branch Dec 24, 2019
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

2 participants