monome / softcut-lib

sample cutting library
GNU General Public License v3.0
35 stars 6 forks source link

simultaneous loop set and position move bug #41

Closed tehn closed 3 years ago

tehn commented 3 years ago

i feel like i've stumbled onto a bug, so logging it here before i look at the softcut code (tomorrow)

ex: if looping from 0-2 and move the loop points to 2-4 and the position to 3 (ie, mid-loop) somehow the position lands on 2, not 3.

likewise if loop is at 2-4, setting it to 0-2 and position to 1 (mid-loop) will actually put position at 0 (loop start).

test code below shows this (use K2 and K3 to set loop regions, feedback is visual from the phase callback)

function init()
  softcut.enable(1,1)
  softcut.buffer(1,1)
  softcut.loop(1,1)
  softcut.loop_start(1,0)
  softcut.loop_end(1,2)
  softcut.position(1,0)
  softcut.play(1,1)

  softcut.phase_quant(1,0.0625)
  softcut.event_phase(update_positions)
  softcut.poll_start_phase()
end

pos = {}

function update_positions(n,d)
  pos[n] = d
  redraw()
end

function redraw()
  screen.clear()
  screen.move(20,10)
  screen.line_rel(pos[1]*16,0)
  screen.stroke()
  screen.move(0,50)
  screen.font_face(2)
  screen.text(pos[1])
  screen.update()
end

function key(n,z)
  if n==2 and z==1 then
    softcut.loop_start(1,0)
    softcut.loop_end(1,2)
    softcut.position(1,1)
  elseif n==3 and z==1 then
    softcut.loop_start(1,2)
    softcut.loop_end(1,4)
    softcut.position(1,3)
  end
end
catfact commented 3 years ago

can reproduce.

this is somehow arising from the crossfade "queueing" behavior... for whatever reason, the incorrect position is getting dequeued instead of the correct position.

i'll try ensuring that queue state is cleared when changing loop positions. (i think this seems like a generally sensible thing to do.)

catfact commented 3 years ago

ok, the mechanism of the bug is pretty clear. this output is demonstrative. here, the loop from [0, 2] is running and then the new loop position of [2, 4] is set, followed by a move to pos=3.

this is all on the audio thread.

first the commands are handled:

handleCommand(): set loop end: 2
handleCommand(): set loop end: 4
handleCommand(): set position: 3
enqueuing crossfade: 144000
done with pending commands

then, the head is proccessed:

subhead looping with phase = 57598
looping to position: 96000
enqueuing crossfade: 96000
dequeuing crossfade: 96000
setting phase for new active head: 96000

line-by-line: subhead looping with phase = 57598

the current phase is outside of the new loop points. this is a necessary condition for the bug.

enqueuing crossfade: 96000 this is where the actual bug happens. the loop logic kicks in and decides we need to jump to the loop start point. what's not obvious is that our "queue" of position changes is just one value deep, and this clobbers the position change requested in the command handler.

(the Q is intended to just handle the case where a position change is requested during a xfade, delaying the change until the completion of the fade. if additional changes come in during the fade, the most recent one "wins." it doesn't try to queue up a bunch of position changes, which would sound weird.)

i have to think a little about how to fix this. i think probably the cleanest thing is to make the queue system a little smarter: like, don't actually use the queued slot unless a fade is in progress. (so that the request position change would take place immediately, and then when the block is processed, the phase is not outside of the loop bounds and the loop is not triggered.)

catfact commented 3 years ago

PR #42 appears to fix