Page MenuHomeFreeBSD

mixer: remove volume backwards compat, add % interpretation
ClosedPublic

Authored by kevans on Apr 30 2022, 3:23 AM.

Details

Summary

The current situation is fairly confusing, where an integer is interpreted
as a percent until you slap a decimal on it and magically it becomes an
absolute value.

Let's have a flag day in 14.0 and remove this shim entirely. Setting with
percent can still be useful, so allow a trailing '%' to indicate as such.
As a side effect, we tighten down the format allowed in the volume a little
bit by ensuring there's no trailing garbage after the value once it's
separated into left and right components.

Relnotes: yes

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

pauamma added inline comments.
usr.sbin/mixer/mixer.8
133

Percent of what? Is "70%" the same as absolute 0.7, 0.7 times whatever the current value is, or something else?

usr.sbin/mixer/mixer.8
133

Right, the intended interpretation is "70% of 1.0"

usr.sbin/mixer/mixer.c
358–359

It occurred to me that this is likely wrong, or that rrel needs to be reset later if n > 1. I suspect the right answer is that this should just set lrel = 1; and we should handle rrel = lrel down where we handle v.right = v.left (inside the switch)

I have no objections for this patch, specifying percentage with "%" else it is relative to 1.0.

Let me know when the patch is ready to land.

Christos?

--HPS

pauamma requested changes to this revision.Apr 30 2022, 9:10 AM
pauamma added inline comments.
usr.sbin/mixer/mixer.8
131–136

More readily understandable. (Insert line break where appropriate.)

133–134

Clarify.

140–145

Are +23% or -42% also possible? I would add examples here if so, along the lines of (if I understand the intent correctly):

If the volume is currently 0.707, then`mixer volume '+23%' followed by 'mixer volume '-42%'` will set it first to 0.869, then to 0.486.

This revision now requires changes to proceed.Apr 30 2022, 9:10 AM

There are some rc.d / devd scripts in base which needs fixing. Just
grep -ri mixer /usr/src

Can you include those deltas too, Kyle?

Getting rid of the backwards compatibility which allows for both normalized
floats and the 0-100 OSS values is a good idea.

I think it's important to document that with relatives volume changes, the
percentage still uses 1.0 as a reference. For example, mixer vol=-5% doesn't
mean that the volume is decreased 5% relative to what the volume already was,
but 5% relative to 1.0 (or at least that's what the code does). This could create
some confusion.

usr.sbin/mixer/mixer.8
131–136

Why 6 decimal digits? Since OSS' internal values range from 0 to 100, it makes
sense to use only 2 decimal digits, as we already do. Or am I missing
something?

140–145

I pointed this out on my comment as well. Although it does make sense, I think
-23% being relative to the current volume can be a bit confusing.

kevans marked 6 inline comments as done.

Highlights:

  • Fix devd scripts
  • Fix other examples found in the tree
  • Fix rrel inheriting lrel prematurely
  • Don't allow garbage after %, either

usbhidaction has an example using awk that I simplified to just
setting the absolute value rather a percentage.

Will re-review the manual page as needed once there's consensus on the proper behavior.

usr.sbin/mixer/mixer.8
131–136

That was me trying to clarify "32-bit float", which does give you 6-7 decimal digits IIRC. But maybe I was overly literal here.

140–145

Relative to the current volume feels more natural to me, but then I don't think I've ever used that volume-setting interface, so POLA overall probably dictates "document clearly what the code actually does, not change the code to fit one person's preconceptions".

In D35101#795642, @pauamma_gundo.com wrote:

Will re-review the manual page as needed once there's consensus on the proper behavior.

Which it looks like there is now, upon closer reading.

This revision is now accepted and ready to land.May 1 2022, 2:15 PM

kevans@ Who are we waiting for?

One more bump for good measure. :-)

Sorry, I'll take care of this soon.