Skip to content

Normalizer stereo imaging and performance improvements #1485

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

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

roderickvd
Copy link
Member

Previously, the normalizer integrator and peak were recalculated as if the samples were a mono stream. If actually one channel required limiting and the other not, this could cause nervousness in the stereo imaging.

This PR changes the normalizer to:

  • calculate the integrator and peak for each channel individually
  • then couple the left/right channel by using the limit gain of each interleaved pair of samples
  • be faster by optimizing the hot code path and reducing the number of branches

Again, please test this for me as I don't have a Spotify account anymore. I ported it from pleezer with my permission.
My "go to" track to test limiting is "Blumine" by Mahler on "Masterpieces in Miniature".

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

playback/src/player.rs:1611

  • [nitpick] Consider renaming 'normalisation_channel' to something like 'current_channel' (or adding a clarifying comment) to better convey its purpose as a toggling index for stereo channels.
self.normalisation_channel ^= 1;

playback/src/player.rs:1758

  • Review the change in the state match pattern: switching from 'PlayerState::Invalid { .. }' to 'PlayerState::Invalid' may bypass handling of extra data previously matched; ensure that this change correctly covers all intended invalid states.
if matches!(self.state, PlayerState::Invalid) {

@photovoltex
Copy link
Member

So I used the state of the branch for a bit (with normalization enabled) and it seems to not have (at least for me) noticeable interference or audio spikes. I will test it for a day more and then we could probably get it merged :D

Copy link
Member

@photovoltex photovoltex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, testing wise I would approve these changes. Just two small comments that didn't make quite a lot of sense to me.

Copy link
Member

@photovoltex photovoltex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah dang it, forgot to approve it sooner. After the conflict is resolved you can merge it :)

@roderickvd
Copy link
Member Author

No prob I'll make some minor changes anyway.

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

Successfully merging this pull request may close these issues.

2 participants