Page MenuHomeFreeBSD

gsoc-2021 final review: sound mixer improvements
ClosedPublic

Authored by christos on Aug 22 2021, 5:50 PM.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

christos created this revision.
NOTE: The kernel changes in this diff are already in 14-current.
mixer_lib/Makefile
8 ↗(On Diff #94024)

What requires "-lm" ?

mixer_lib/mixer.h
35 ↗(On Diff #94024)

Why do you need to include <math.h> ?

christos added inline comments.
mixer_lib/Makefile
8 ↗(On Diff #94024)

The MIX_VOLDENORM macro.

mixer_lib/mixer.h
35 ↗(On Diff #94024)

For the MIX_VOLDENORM macro.

christos marked 2 inline comments as done.

Removed useless files.

mixer_lib/mixer.h
68 ↗(On Diff #94052)

Can we change this to:

#define MIX_VOLDENORM(v)	((int)((v) * 100.0f + 0.5f))

roundf() handles some special cases like negative values and infinite values. I think we don't need that.

Then we can get rid of the libm dependency and math.h .

mixer.3: updated MIX_VOLDENORM definition.

mixer_lib/mixer.h
26 ↗(On Diff #94166)

__BEGIN_DECLS

Should be after the include of sys/cdefs.h

The FBSDID is only for C-files. Please remove and change it into:

  • $FreeBSD$

At the end of the copyright notice. See other files for an example.

36 ↗(On Diff #94166)

I would prefer 1U << (n) instead of 1 << n

christos marked 2 inline comments as done.

Added $FreeBSD$ tags.

dmitryluhtionov_gmail.com added inline comments.
mixer_lib/mixer.c
264 ↗(On Diff #94166)

Post-assignment check

mixer_lib/mixer.h
104 ↗(On Diff #94166)

Why mixer_close() return any value ?
In all the code it is called as (void)mixer_close(m);

mixer_prog/mixer_prog.c
289 ↗(On Diff #94166)

dunit not a unsigned integer

349 ↗(On Diff #94166)

mixer_get_ctl() may return NULL
cp->name - can be deference pointer

mixer_prog/mixer_prog.c
289 ↗(On Diff #94166)

maybe it makes sense to make it like unsigned int ?

christos marked 3 inline comments as done.

mixer(3): fixed post assignment check in mixer_remove_ctl.
mixer(8): added error handling in initctls.

mixer_lib/mixer.c
264 ↗(On Diff #94166)

Fixed.

mixer_lib/mixer.h
104 ↗(On Diff #94166)

The library is meant to be used by mixer programs in general, so other
programs might want to check the return value of mixer_close().

mixer_prog/mixer_prog.c
349 ↗(On Diff #94166)

Edited initctls() so we can continue only if everything went correctly in the control initialization.

mixer_prog/mixer_prog.c
316 ↗(On Diff #94236)
mix_ctl_t *
mixer_get_ctl(struct mix_dev *d, int id)
{
	mix_ctl_t *cp;

	TAILQ_FOREACH(cp, &d->ctls, ctls) {
		if (cp->id == id)
			return (cp);
	}
	errno = EINVAL;

	return (NULL);
}

below

	warn("%s.%s=%.2f:%.2f", 
			    m->dev->name, cp->name, v.left, v.right);

without any checks

mixer_prog/mixer_prog.c
316 ↗(On Diff #94236)

warn() is called in case mixer_set_vol() fails -- this doesn't
have to do with what mixer_get_ctl() returns. As I said in the
previous comments, these calls to mixer_get_ctl() are
guaranteed to work since error checks are done in inictls.

I'm pulling this in. Let's see what happens.

This revision is now accepted and ready to land.Wed, Sep 22, 1:08 PM