Processing interface cleanup#78
Conversation
…nput buffer copy.
… call for "benchmodel" tool.
|
Awesome! Based on the description, this sounds super. I'll have a look when I get a chance. |
sdatkinson
left a comment
sdatkinson
left a comment
There was a problem hiding this comment.
LGTM! 👍🏻
Thanks for this PR!
| @@ -107,7 +100,7 @@ class DSP | |||
| virtual void _process_core_(); | |||
|
|
|||
| // Copy this->_core_dsp_output to output and apply the output volume | |||
There was a problem hiding this comment.
Nit: Since we're getting rid of gain, might be able to simplify this even more.
| { | ||
| this->_get_params_(params); | ||
| this->_apply_input_level_(inputs, num_channels, num_frames, input_gain); | ||
| this->_input_samples = input; |
There was a problem hiding this comment.
Nit: I bet we can simplify this even more: I think that process() can be a pure virtual function.
| // Default implementation is the null operation | ||
| for (size_t i = 0; i < this->_input_post_gain.size(); i++) | ||
| this->_core_dsp_output[i] = this->_input_post_gain[i]; | ||
| for (size_t i = 0; i < _num_input_samples; i++) |
There was a problem hiding this comment.
Nit: yeah, I'm probably going to PR to put this as the default implementation of process() and get rid of _process_core_() entirely 🙂
| for (int c = 0; c < num_channels; c++) | ||
| for (int s = 0; s < num_frames; s++) | ||
| outputs[c][s] = (NAM_SAMPLE)(finalGain * this->_core_dsp_output[s]); | ||
| const double finalGain = this->mNormalizeOutputLoudness ? loudnessGain : 1.0; |
There was a problem hiding this comment.
Hmm, I see.
Question: What do you think about having this class not be responsible for normalizing? It can provide information, but leave it up to the plugin to do anything about it? I'm happy either way, but perhaps a tiny bit happier pushing it out of this library because of how it'd clean up all of the subclasses and make it a little simpler from a dev standpoint I think.
There was a problem hiding this comment.
Yep, that was my preference from the beginning: #18 (comment) 😉
I think it makes sense for the plugin to do it, since it will already be doing output gain adjustment - so no additional pass necessary.
|
Overall, I agree that there is more simplification/restructuring that can/should be done. I avoided going too far at once in the interest of making some quick progress that could easily be merged. |
2 participants
This PR cleans up and simplifies the processing interface by doing the following:
The process call is now just simply:
void DSP::process(NAM_SAMPLE* input, NAM_SAMPLE* output, const int num_frames)This mostly is just a benefit for simplicity of interface, although for simple models like LSTM, it might make for a small noticeable performance increase.
@sdatkinson - note that I just removed the model parameter dictionary from the process call. I didn't take out the internal code using it - I figured that is better done by you.
I've tested WaveNet and LSTM, but not ConvNet.
Resolves #77
Resolves #27
Gives us a start on #49