MWEngine icon indicating copy to clipboard operation
MWEngine copied to clipboard

looping backward

Open scar20 opened this issue 3 years ago • 11 comments

After staring and procrastinated at sampleevent.ccp and baseaudioevent.cpp I finally decided to give a try. I first made a bool member in sampleevent - isForward - with a setter void setPlaybackDirection(bool forward). In the mixBuffer() method, I commented out the if ( !_loopeable ) and added a if (_isForward) { wrapping the first for loop, and "elsing" the same block of code this time changing the last lines

if (++_readPointer > maxReadPos) { _readPointer = _loopStartOffset; sampleLoopStarted = true; }

to

if (--_readPointer < minReadPos) { _readPointer = _loopEndOffset; sampleLoopStarted = true; }

and it didn't work as I was expecting... But then I though that this was for the sequencer and I only use LivePlayback. So I did the same inside getBufferForRange changing the equivalent lines of code:

if (--_rangePointer < _bufferRangeStart) _rangePointer = _bufferRangeEnd;

This time it worked, I didn't expect that since its rare I get something going on first try. So I did the same again for the custom playback speed and it still worked. In fact, it work too well... Too easy... Even changing the direction while playing just do that. I don't really understand how that can be... After all, we are in a for loop and I can't see how changing the outside "if" can influence what is already happening inside. What I was expecting is that it finishes first its loop before playing with the new values bit I am clearly wrong (not that I complain...). I could say the same for the playback rate. So here I'm a bit baffled. Is this threaded? As you can see, I'm still trying to get my head around the code, mapping my way around.

You can have a look here - I added a forward/backward button beside the play button https://bitbucket.org/androfone/filtertest/src/loop/

scar20 avatar May 06 '21 11:05 scar20

OK, I think I got it, Those for (int i = 0; i < bufferSize; ++i) loop are not sample length size, just the standard input "chunk" buffer, ahhhh... I'm still confused by all the differents buffers and the interaction with sequencer, I want to implement right, not just hacking for my need.

scar20 avatar May 06 '21 17:05 scar20

Hi!

Yes there is internal buffer management happening in SampleEvent, also with fractional increment and interpolation (as playback speed can be altered to preserve pitch at different sample rates). Additionally alternate ranges can also be specified (to create loops from a small subsection of a sample where the tail is only played when the event is released, similar to a SoundFont).

That does make for a bit of housekeeping, to add reverse playback kinda puts that upside down (heh). However, I think this is a nice feature to add to the ones I listed above. I'll put it in the TODO list, I think this can be solved elegantly but I need to wrap my head around on how to tackle this, probably unit test first and implementation later.

igorski avatar May 10 '21 18:05 igorski

RIght now, I have done almost all the cases. Added a setter for internal variable "isForward". Done implementation in getBufferForRange case playback = 1.0 and playback = custom. Ongoing implementation in mixBuffer: if(playbackRate == 1.0) { if(!loopeable) { // commented out for now loopeable implementation done return; } // custom playbackrate if (!_loopeable) { implementation not done } else { implementation done }

So there is only the !loopeable case left to do. I've begun to test with a sequence at this moment. I must say that the naming convention (hum...) gave me hard time... buffer and _buffer, fReadPointer, _readPointerF, loopOffset, _loopEndOffset, _loopStartOffset, etc... That was quite confusing. When I'll have all running, I maybe put some suggestions in comments for some name refactor. Anyhow, I'll send you the implementation once done so you can evaluate.

Last thing; I have put the if(isForward) test outside the for(buffer) loop since I try to avoid putting more if's inside those loops but the end result is making that class monstruosly bigger. On the other side, having them inside mean testing at each sample. So I wonder about efficiency versus readability. I plan also to implement back and forth looping: sample start loopstart loopend o---------------------------->|<---------------------------->| So another test...

Lastly, once everyting work ok, I want to look for parameters smoothing - distributing amount of parameter change (speed) at the sample level - since I noticed some "zipping" artefacts if you change the parameter very fast. That could be usable for others effects as well.

scar20 avatar May 11 '21 01:05 scar20

buffer and _buffer, fReadPointer, _readPointerF, loopOffset, _loopEndOffset, _loopStartOffset, etc... That was quite confusing.

The underscore prefix implies the class level value whereas the one without is a function level value (which for some values like the read offsets might imply updating the class level values for the next iteration at the end of the function). But yeah I can see the confusion (fReadPointer and readPointerF for instance).

I have put the if(isForward) test outside the for(buffer) loop since I try to avoid putting more if's inside those loops but the end result is making that class monstruosly bigger. On the other side, having them inside mean testing at each sample. So I wonder about efficiency versus readability.

That is why the SampleEvent contains the largest functions in the codebase, it executes code at the rendering level and therefor benefits from speed (meaning everything that can be calculated once or ahead of time should do so). I believe such operations should not come at the expense of performance, though often there is a tradeoff with maintenance (as you now experience).

I am most appreciate of you (once more) doing a lot of heavy lifting here. When you're ready - or let me know if I can help you with the final part regarding looping or anything else - I can have a look at optimization of the code. In the "worst" case we can create inline functions which have the benefit that they provide an isolated example for each purpose (e.g. looped playback, looped backwards playback, alternate playback rate forwards/backwards, etc.) while at compile time they generate sequential code inlined with the sample read instructions.

igorski avatar May 12 '21 17:05 igorski

Ok, done all cases. There is still crossfading to do and also the "fmod" part which I don't understand (I kinda guess but I just don't understand what fmod do exactly).

It look like case livePlayback is not handled for !loopeable (oneShot) or perhaps it is intrinsic.

I have done a revised (but messy...) MainActivity to showcase de differents options with or without a sequence - I messed with your sequence since it reminded me Alan Parson I Robot... So there is a one shot sample forward or reverse and a lopped sample forward or reverse with the sequence. There is a conceptual problem though with setting loop start & end on the reversed sample in the sequence; I user might use a beat metric to set the start/end point which is fine in forward but is not the same in reverse (I falled in that trap before realizing it). Perhaps an explicit setter for setting loop by beat could be added. For now the two sliders handling this sample do nothing.

The upper part of the activity is livePlayback range based loop - there is a quirk though: when changing sample, pressing play/pause restart the sample at the last position instead of from the beginning. You must displace the Range Start slider so it restart then from the beginning. Need to investigate. The middle part is livePlayback looping without range - always restart from beginning (or end reversed) on play then loop from loop start point (end if reversed). The bottom part is the sequence Start/stop with synths-drums & samples button with buttons for directions of both oneshot and looped sample and below controls for speed and volume.

I pushed this version in the repository: https://bitbucket.org/androfone/filtertest/src/loop/

Once the code is 100% correct, then we can think refactoring and optimization - I was thinking of an iterator that can be flipped +1, -1 and independent loop boundary that could be also flipped. I was begin to look at pointer functions to "inject" the proper tests at some crucial point but I don't know if that could work - you'll know better than me for what to do next.

scar20 avatar May 18 '21 22:05 scar20

Added crossfade inversion which seems to be OK on hearing but I don't know how to debug/monitor at sample level. Its on the Looping sample - the mid-section of the app - and its set to 100ms.

As for the fmod thing, it look its a residual added to the read pointer when it get back to start point, so I just subtracted the same amount when it get back to the loop end point when played backward.

Removed also left over code from my parameter smoothing experiment - that will be another thread.

Just pushed updated version at same address: https://bitbucket.org/androfone/filtertest/src/loop/

scar20 avatar May 19 '21 16:05 scar20

Ah excellent! I'll be looking into this next and extending the tests to verify its intended functionality and optional regressions.

igorski avatar May 20 '21 16:05 igorski

Well, I just noticed some "sizzling" on reverse looping with custom speed - OK when 1:1. I compared with range loop with custom speed ; range loop is OK. So I'll have to look further in the "custom speed" !isForward part of mixBuffer() and find what is wrong.

scar20 avatar May 20 '21 21:05 scar20

OK, it was in the interpolation part of custom rate nonloopeable and custom rate loopeable. I inverted t and t2 as t2 = t - 1 instead of t2 = t + 1 in the forward mode. Reverted back to t + 1 fixed it so I deduced it was the last part tgtBuffer[i] += ((s1 + (s2 - s1) * frac) * _volume); s2 and s1 must been flipped too as s1 - s2 instead. That was it. Pushed the corrections at same address.

scar20 avatar May 21 '21 14:05 scar20

I noted two bugs in SampleEvent: 1 - if range based and looping (forward) with speed below 1:1, when the pointer reach the end, it crash. I suspected inaccuracy in the method that provide bufferRangeEnd in this test inside SampleEvent::getBufferForRange()

if (_rangePointerF += _playbackRate > bufferRangeEnd)
    _rangePointerF = (float) _bufferRangeStart;

I was able to get away with it by testing with the min of bufferRangeEnd and eventEnd

float max =  std::min(bufferRangeEnd, (float)eventEnd);
if ((_rangePointerF += _playbackRate) > max)
    _rangePointerF = (float) _bufferRangeStart;

2 - Still using range, the sample don't get reset at the beginning on successive play(), it continue where it was left. This is because there is a missing _rangePointerF (used inside getBufferForRange()) assignment in the play() method. This was an easy fix (but hard to find) bug, just added it to the list of assignment:

_readPointer = std::max(0, _bufferRangeStart);
_readPointerF = (float) _readPointer;
_rangePointerF = _readPointerF; // seem this is the fix for range not reset to start on play
_lastPlaybackPosition = _bufferRangeStart;

About the rest of this ongoing work, right now I'm in a rush to produce the app so I've made SampleEvent to just use range and always use custom rate (since there is some gliches when changing speed from 1:1), skipping all the cases but two. Later on, I want to make a pull request with this work once I've cleaned up the code and test all case right with a test case app.

scar20 avatar Dec 18 '21 18:12 scar20

I was beginning to make a mess with the class... So to keep sanity, I created a branch "mess", then created a class SampleEventRange which is a stripped down version of SampleEvent only dealing with range play in custom rate forward/backward oneshot/loop. All original bool SampleEvent::getBufferForRange( AudioBuffer* buffer, int readPos ) is now compacted into void SampleEventRange::mixBuffer(AudioBuffer *outputBuffer, int playbackPos, int minPos, int maxPos) the two last parameter not even used. Its messy... But it work!

Now I assume that the variable _lastPlaybackPosition that get passed to the original inner mixBuffer( outputBuffer, _lastPlaybackPosition, 0, getBufferRangeLength(), false, 0, false ); is meant to always go forward to sync with sequencer. Let me know if I'm right or wrong about this.

Now I dont do that; to correctly duplicate the behavior in Fonofone, I've make it reversable so that oneshot can be reversed while playing and stopped only when reaching bounds instead of stopping at play length.

You can take a look at your leisure https://github.com/scar20/MWEngine/tree/mess which I actually use for the build. I will have to carefully review all the cases to integrate these changes cleanly in SampleEvent when I have more time.

scar20 avatar Dec 29 '21 20:12 scar20