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

Fix chorus not being rendered when an audio processing callback is used. #751

Closed

Conversation

chirs241097
Copy link
Contributor

in fluid_rvoice_mixer.c:fluid_rvoice_mixer_process_fx():

    // all dry unprocessed mono input is stored in the left channel
    fluid_real_t *in_rev = fluid_align_ptr(mixer->buffers.fx_left_buf, FLUID_DEFAULT_ALIGNMENT);
    fluid_real_t *in_ch = in_rev;

    fluid_profile_ref_var(prof_ref);


    if(mix_fx_to_out)
    {
        // mix effects to first stereo channel
        out_ch_l = out_rev_l = fluid_align_ptr(mixer->buffers.left_buf, FLUID_DEFAULT_ALIGNMENT);
        out_ch_r = out_rev_r = fluid_align_ptr(mixer->buffers.right_buf, FLUID_DEFAULT_ALIGNMENT);

        reverb_process_func = fluid_revmodel_processmix;
        chorus_process_func = fluid_chorus_processmix;

    }
    else
    {
        // replace effects into respective stereo effects channel
        out_ch_l = out_rev_l = fluid_align_ptr(mixer->buffers.fx_left_buf, FLUID_DEFAULT_ALIGNMENT);
        out_ch_r = out_rev_r = fluid_align_ptr(mixer->buffers.fx_right_buf, FLUID_DEFAULT_ALIGNMENT);

        reverb_process_func = fluid_revmodel_processreplace;
        chorus_process_func = fluid_chorus_processreplace;
    }

If an audio processing callback is used, mix_fx_to_out would be FALSE. As a result, in_ch and out_ch_l points to the same buffer.

in fluid_chorus.c:fluid_chorus_processreplace():

        /* process stereo unit */
        /* store the chorus stereo unit d_out to left and right output */
        left_out[sample_index]  = d_out[0] * chorus->wet1  + d_out[1] * chorus->wet2;
        right_out[sample_index] = d_out[1] * chorus->wet1  + d_out[0] * chorus->wet2;

        /* Write the current input sample into the circular buffer */
        push_in_delay_line(chorus, in[sample_index]);

Here the chorus processing code writes to the left output buffer (which will overwrite the input buffer in this case) before the sample from the input buffer is stored into the delay buffer, making the chorus output all zeros. If no audio processing callback is used, mix_fx_to_out would be TRUE and in and left_out will not point to the same buffer, therefore the order doesn't matter.

Simply swapping the two steps should be a sufficient fix. This patch also apply the same change to fluid_chorus_processmix only for the sake of consistency (since they are almost exact copies of each other).

@derselbst
Copy link
Member

Ouch, chorus being broken for fluid_synth_process since 2.1.0 is no good news. The fix you've provided seems correct to me, thank you! The broken unit test is caused by a denormal float being processed in the chorus delay line. Didn't happen before, because it obviously kept pushing zeros to the delay line.

I'll cherry-pick this to the 2.1.x branch. Unfortunately, I see no good way to unit-test this, without injecting too much mock code into reverb and chorus.

@jjceresa FYI

@derselbst derselbst added this to the 2.1 milestone Jan 23, 2021
@derselbst derselbst added the bug label Jan 23, 2021
derselbst added a commit that referenced this pull request Jan 23, 2021
@derselbst
Copy link
Member

Just tested manually and confirmed that it has been fixed. Thanks!

@derselbst derselbst closed this in 8e9d361 Jan 23, 2021
@jjceresa
Copy link
Collaborator

The fix you've provided seems correct to me too. However, I suggest that a comment should be added to be aware of the reason of this fix. For example:

/* Write the current input sample into the circular buffer.
This must be done before "processing stereo unit" (below).
This ensures input buffer not being overwritten by stereo unit output in the case
of the provided 'in' buffer is the same as 'left_out' or 'right_out' buffer.
*/

@derselbst
Copy link
Member

Will do.

derselbst added a commit that referenced this pull request Jan 23, 2021
jet2jet pushed a commit to jet2jet/fluidsynth-emscripten that referenced this pull request May 27, 2021
…lay line.

In `fluid_rvoice_mixer.c`:`fluid_rvoice_mixer_process_fx()`:

If an audio processing callback is used, `mix_fx_to_out` would be `FALSE`. As a result, `in_ch` and `out_ch_l` points to the same buffer.

In `fluid_chorus.c`:`fluid_chorus_processreplace()`:
```C
        /* process stereo unit */
        /* store the chorus stereo unit d_out to left and right output */
        left_out[sample_index]  = d_out[0] * chorus->wet1  + d_out[1] * chorus->wet2;
        right_out[sample_index] = d_out[1] * chorus->wet1  + d_out[0] * chorus->wet2;

        /* Write the current input sample into the circular buffer */
        push_in_delay_line(chorus, in[sample_index]);
```

Here the chorus processing code writes to the left output buffer (which will overwrite the input buffer in this case) before the sample from the input buffer is stored into the delay buffer, making the chorus output all zeros. If no audio processing callback is used, `mix_fx_to_out` would be `TRUE` and `in` and `left_out` will not point to the same buffer, therefore the order doesn't matter.

Simply swapping the two steps should be a sufficient fix. This patch also apply the same change to `fluid_chorus_processmix` only for the sake of consistency (since they are almost exact copies of each other).

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

Successfully merging this pull request may close these issues.

None yet

3 participants