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

Fix sndio raw_volume rounding #2226

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jzavrk
Copy link

@Jzavrk Jzavrk commented Mar 10, 2025

fix #2225

Fixes + - keys in ncmpc

@MaxKellermann
Copy link
Member

This PR contains text that is missing in the commit message; and the PR text isn't even enough because it doesn't describe what is being done and what the problem was.
Also this can be done better than reverting to floating-point math. Dividing integers with rounding is trivial enough.

@Jzavrk
Copy link
Author

Jzavrk commented Mar 10, 2025

I expressed myself badly, this PR addresses mainly this issue: MusicPlayerDaemon/ncmpc#95
Floating-point math is necessary since dividing 2 integers gives no more than floor value, rendering increment impossible.
This patch has been tested before being sent to review, volume now goes effectively up and down.

@MaxKellermann
Copy link
Member

No, floating-point math is not necessary! You don't believe me, so I have to prove it.

Look this piece of code:

return std::round(static_cast<double>(raw_volume) * 100 / SIO_MAXVOL)
  1. converts integer to floating point
  2. floating point multiplication by 100
  3. floating point division by 127
  4. invoke function round()
  5. convert floating point to integer

Now compare with this one:

return (raw_volume * 100 + SIO_MAXVOL / 2) / SIO_MAXVOL
  1. integer multiplication by 100
  2. integer addition, adding 63
  3. integer division by 127

Both functions are (almost) equal, but your version is bigger and much slower.

@Jzavrk
Copy link
Author

Jzavrk commented Mar 10, 2025

Tested, it works 👍

@MaxKellermann
Copy link
Member

Your PR now contains 2 commits, the first of which is not good. I don't want to merge known-bad commits. If you find a mistake in an unmerged commit, you should amend the commit instead of adding fixup commits.

@Jzavrk Jzavrk force-pushed the sndio-volume-bug-fix branch from 3688752 to 1e151a6 Compare March 10, 2025 18:23
@Jzavrk
Copy link
Author

Jzavrk commented Mar 10, 2025

Forced-pushed the amended commit, I hope this one will do it.
I don't know if it has its place here but I haven't been into coding since studies (years now), first PR ever, setting up for testing on my linux desktop was not too easy (I use mpd on my FreeBSD server).
I would be sincerely curious to know what was the reasoning behind the new formulas, sounds like magic to me

@MaxKellermann
Copy link
Member

Code is good now, but please write a proper commit message which explains the problem and how it is being solved.

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.

Rounding error setting volume using sndio output
2 participants