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

Output loudness normalization #18

Merged
merged 5 commits into from
Apr 2, 2023
Merged

Output loudness normalization #18

merged 5 commits into from
Apr 2, 2023

Conversation

sdatkinson
Copy link
Owner

Implements reading the "loudness" metadatum (if available) as well as output normalization.

Resolves #16
Resolves #17

@sdatkinson sdatkinson linked an issue Apr 2, 2023 that may be closed by this pull request
@sdatkinson sdatkinson marked this pull request as draft April 2, 2023 06:03
@sdatkinson
Copy link
Owner Author

Waiting for #13 and #15

@mikeoliphant
Copy link
Contributor

Would it maybe make sense just to expose the loudness to the plugin as a property of the model, and let the plugin apply the gain at the same time as it does the output leveling?

I was hoping to actually get rid of the gain adjustment step completely in the NAM code, since it isn't currently doing anything - this would allow us to remove an extra buffer copy. Not super significant compared to the model simulation itself, but every little bit helps.

@olilarkin
Copy link
Contributor

olilarkin commented Apr 2, 2023

I am not sure but I think there is quite a bit of redundant buffer copying code in the NAM plugin src, an issue for another day (and another repo)

@sdatkinson
Copy link
Owner Author

Would it maybe make sense just to expose the loudness to the plugin as a property of the model, and let the plugin apply the gain at the same time as it does the output leveling?

I was hoping to actually get rid of the gain adjustment step completely in the NAM code, since it isn't currently doing anything - this would allow us to remove an extra buffer copy. Not super significant compared to the model simulation itself, but every little bit helps.

Not sure. Here's my thinking on it:

  • Whether the model's output should be normalized feels like a mode of operation for the model object itself. Which is why I think it makes sense for it to own it & be used via an abstraction like SetNormalize().
  • Some models won't have this information available. Instead of pushing that complexity & handling out to the code using the model, SetNormalize() keeps it as a matter that's internal to the model. One could output the "target loudness", but it wouldn't be clear whether the model actually was this loud or if it was just outputting a "placeholder". If you're thinking of something like double GetLoudness(), happy to hear you out.
  • FWIW, this doesn't introduce an additional copy (just uses the one that's there), and the implementation of applying normalization can easily be changed to being part of _process_core_() in a number of ways. At least the current implementation should add a negligible amount of compute over the previous code. ATM, my thinking is "[...] premature optimization..."--so if there really is a performance boost and you want it, I'm happy to field a PR that changes the implementation. I suspect it'll add complexity to the codebase if I code it up, but perhaps yours would be nice & clean.

Happy to hear both your thoughts.

@sdatkinson sdatkinson marked this pull request as ready for review April 2, 2023 19:13
@mikeoliphant
Copy link
Contributor

  • FWIW, this doesn't introduce an additional copy

It doesn't, no, but it does prevent removing the existing copy at both input and output for gain adjustment (that isn't currently required, because the plugin handles I/O gain adjustment).

My plan was to completely get rid of "_input_post_gain" and "_core_dsp_output" and read/write directly from the buffer pointers passed to process. Removes two copies.

@sdatkinson
Copy link
Owner Author

  • FWIW, this doesn't introduce an additional copy

It doesn't, no, but it does prevent removing the existing copy at both input and output for gain adjustment (that isn't currently required, because the plugin handles I/O gain adjustment).

It doesn't prevent it. The implementation of applying loudness could simply involve applying the gain at the point where the core dsp writes to the buffer. Or, an in-place multiplication could be done, which might read a little cleaner, but also might not compile-time optimize.

My plan was to completely get rid of "_input_post_gain" and "_core_dsp_output" and read/write directly from the buffer pointers passed to process. Removes two copies.

Agreed--this is a good thing to do.

@mikeoliphant
Copy link
Contributor

The implementation of applying loudness could simply involve applying the gain at the point where the core dsp writes to the buffer.

True. Still one extra multiplication pass over having the plugin apply it with output level, though. But I agree that this get into small potatoes optimization, so I'll shut up about it for now :-)

@sdatkinson sdatkinson merged commit c0d0c2c into main Apr 2, 2023
@sdatkinson sdatkinson deleted the issue-16 branch April 2, 2023 19:31
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.

Output loudness normalization Store loudness information in model
3 participants