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

Get rid of DSP::_process_core_() and DSP::_apply_output_level_(), make process() do what _process_core_() does #79

Closed
sdatkinson opened this issue Oct 6, 2023 · 3 comments · Fixed by #80
Assignees
Labels
enhancement New feature or request

Comments

@sdatkinson
Copy link
Owner

While reviewing #78 I realized that the DSP class's structure might be somewhat simplified by making process() do what _process_core_() does currently. In most cases, the existing _process_core_() implementations could become the implementations for process() directly with minimal modifications (e.g. add three lines to assign the pointers and buffer length before proceeding with the rest).

However, the current _apply_output_level_() is taking care of output level normalization; if we want this to remain part of this lbirary's responsibility, then the current class structure is a nice and DRY solution, but I can also see the justification of pushing this out of DSP and letting plugins using NAM take care of it themselves (getting the information they want via e.g. a new DSP::GetLoudness().

I'm somewhat keen to do the latter (simplify this library and push out the gain adjustment to be the responsibility of the plugin) but want to raise it as an issue here to see if anyone has any strong opinions or thoughts if I'm missing something.

@sdatkinson sdatkinson added the enhancement New feature or request label Oct 6, 2023
@sdatkinson
Copy link
Owner Author

Related: #58

@mikeoliphant
Copy link
Contributor

👍 on pushing the loudness adjustment out to the plugin.

If we're in agreement, feel free to assign this to me and I'll put together a PR.

@sdatkinson
Copy link
Owner Author

Sounds great! Thanks 🙂

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

Successfully merging a pull request may close this issue.

2 participants