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

Alter library to support streaming from a ringbuffer #62

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gnif
Copy link

@gnif gnif commented Mar 21, 2019

This change allows an application to stream from non contiguous memory avoiding the need for an intermediate buffer. For example:

size_t need;

while((need = rnnoise_need_samples(state)) && buffer_available() >= need) {
  float * samples;
  size_t available = buffer_get_contiguous(need, &samples);
  rnnoise_add_samples(state, samples, available);
}

if (need == 0)  {
  rnnopise_process_frame(state, output);      
  // process the output
}

@wegylexy
Copy link
Contributor

But wouldn't this prohibits in-place processing?

@gnif
Copy link
Author

gnif commented Jun 29, 2020

All this change does is move the x[FRAME_SIZE]; from a local on the stack to static memory allowing the state to be retained between calls, there is no change to the processing logic other then allowing us to "continue" when there was an incomplete buffer (ie, ring buffer wrap around).

@wegylexy
Copy link
Contributor

In your example, you read from one buffer and output to another. How to transform the following case to fit your changes?

buffer_up_to_480(same);
rnnopise_process_frame(state, same, same); // in the old API, one buffer can both be input and output

or

// in a real-time audio graph, a node has equal input and output size that there will be glitches if not fully filled
if (buffered - input > latency)
    rnnoise_process_frame(state, &ring[(input - latency + size(ring)) % size(ring)], &ring[input]);

@wegylexy
Copy link
Contributor

wegylexy commented Jun 29, 2020

It would be great to support the following use cases by simply rnnoise_process_frame(state, same, same, same_size).

Case 1: fixed I/O buffers, 128 frames each; intrinsic latency of 640 frames is achieved with a ring buffer of 1920 frames
https://github.com/wegylexy/rnnoise_wasm/blob/ea062b216ae3e851930b036eabb235140a87dd0d/src/worklet.c

Case 2: variable I/O buffers up to 16384 frames each, input feed and output demand may change but are always equal
https://github.com/wegylexy/rnnoise_wasm/blob/899060eb011b6e95ad70c4dabf475b08e23be2d2/src/worklet.c

@gnif
Copy link
Author

gnif commented Jun 30, 2020

It can still be done exactly the same, what you are asking for can still be achieved with it, the difference is that we can have a ring buffer that wraps around so the data is not linear.

Here is your use case using this API:

buffer_up_to_480(same);
rnnoise_add_samples(state, same, available);
rnnopise_process_frame(state, same); 

This change simply allows rnnoise to retain the state when it has been given < 480 samples and was only able to partially process the buffer.

Please review the rnnoise source, this does not add any extra buffering or latency. Rnnoise will only process audio in blocks of 480 samples, no more or less. This API change simply allows one to feed it this data in blocks/segments if the source is not providing them at this rate (ie, a resampler may yield odd block sizes) instead of introducing an additional buffer.

@wegylexy
Copy link
Contributor

Currently, the following example requires an intermediate buffer between WebAudio and RNNoise:
https://github.com/wegylexy/rnnoise_wasm/blob/899060eb011b6e95ad70c4dabf475b08e23be2d2/src/worklet.c#L28-L51

Since your changes introduce an internal buffer. Can it eliminate my intermediate buffer above?
Preferably rnnoise_process_frame(state, same, same, length), like pipe(length) in my example.

Pseudo JavaScript:

wasmMemory.set(webAudioInputBuffer); // copy from WebAudio into WASM memory
wasm.pipe(currentBufferLength); // call into C code
// inside WASM: rnnoise_process_frame(state, wasmMemory, wasmMemory, currentBufferLength);
webAudioOutputBuffer.set(wasmMemory.subArray(0, currentWebAudioBufferLength)); // copy back from the same WASM memory

Actual JavaScript: https://github.com/wegylexy/rnnoise_wasm/blob/899060eb011b6e95ad70c4dabf475b08e23be2d2/src/processor.js#L18-L21

@gnif
Copy link
Author

gnif commented Jun 30, 2020

Since your changes introduce an internal buffer

No it does not, read the source! This buffer already exists except it's scoped LOCALLY on the STACK, there is NO additional buffer. Read the source.

biquad(x, st->mem_hp_x, in, b_hp, a_hp, FRAME_SIZE);
vs
biquad(st->input + st->input_pos, st->mem_hp_x, in, b_hp, a_hp, samples);

and

silence = compute_frame_features(st, X, P, Ex, Ep, Exp, features, x);
vs
silence = compute_frame_features(st, X, P, Ex, Ep, Exp, features, st->input);

input already exists, just as a local called x... this change MOVES (not adds) it to the state and tracks how full it is. This is NOT an additional buffer.

Can it eliminate my intermediate buffer above?

No idea, you need to figure that out yourself. This is for when you have a buffer that is not a multiple of 480 samples and "WRAPS AROUND", see: https://en.wikipedia.org/wiki/Circular_buffer

@wegylexy
Copy link
Contributor

Ah, ok. You move it from the stack to the heap.
I understand now, but it does not avoid the need for an intermediate buffer in case output size must equal input size that is not a multiple of 480. Rather, it breaks one line into two, and access the heap which may be on a different memory page than the stack...
How about use different names for your new functions, and keep the current signature of rnnoise_process_frame() for backward compatibility and those who don't need this functionality?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants