Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Allow effect to continue processing if its buffer isn't empty (builtin reverb/delay) #14099
base: main
Are you sure you want to change the base?
Allow effect to continue processing if its buffer isn't empty (builtin reverb/delay) #14099
Changes from 14 commits
ac70a9a
8fe86ec
b3938ea
c8e5222
38f1858
e3cd6bb
29f5317
c1ba962
2153a91
9f2fd6f
a9a679c
1920ffc
29b6fa2
aaf6d09
3654121
ebba883
3f1f385
fe00dbf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does the energy depend on the buffer size? The average shouldn't be influenced by the sample size of the dataset.
If the concern is that the fading tail influences the current average, simply don't sample it. Only sample as much as you need:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The average sample energy depends on the buffer-size since it is computed from the buffer (average over buffer length, the average over the next buffer as well, or only half the buffer, would be numerically different). Thus,
avgSampleEnergy
implicitly depends on the buffer length.Before your previous review, the
averageSampleEnergy
would not return an average but the summed Sample Energy over the buffer, this was simply bad code and is now fixed. To retain the exactly same threshold which was test-driven by me as before, the0.00001f
magic number now needs to be divided by thedelayBufferSize
. Of course I can also shift the0.00001f
a bit by making it 100x smaller or so and call it a day :--) let me knowThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, now that we're only looking at the samples at the beginning of the delay buffer (exactly the samples that feedback into the effect to be exact) the size shouldn't anymore, right? Am I making sense here?
Besides I think
.first(engineParameters.sampleRate())
is probably wrong, my mistake. I meant.first(<how big the current playback buffer is>)
...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhm I am not sure I follow sorry, I think the average is calculated over the whole delay buffer. But we can in any case remove the division by the delay buffer length in the treshhold check. This is just numerics:--)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the solution is not indepenedent of single clicks vs pink noise.
The noise value will contribute way more than a single click.
If we assume that the last added sample was above the threshold, we can alternatively just consume the number samples according to the current picked delay value. Not sure if the earlay disabling has benefits. Does it?
Did you consicder to loop through the whole buffer and continue once you have found a single semale above a certain thresshold?
Maybe copy/reuse the approch that we took here:
mixxx/src/analyzer/analyzersilence.cpp
Line 27 in ae2dc10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you /4 ?
It would be ideal if the feature works the same independent form samplesPerBuffer().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have here two delay filters per channel:
mixxx/lib/reverb/Reverb.cc
Line 216 in ae2dc10
Initalized with 2^6 and 2^8 buffer size. This means the maximum delay is 256 frames.
The issue here in addition is that you are checking the current buffer and not was has added to the reverb.
EffectEnableState::Disabling means that no new samples are added, right?
Cant we just always process additional 256 Samples maybe a few more because of band filters, and than disable?