On Wed, May 11, 2011 at 02:50:36AM +0300, Sviatoslav Chagaev wrote:
>
below are few comments about the diff itself
> Index: aparams.h
> ===================================================================
> RCS file: /OpenBSD/src/usr.bin/aucat/aparams.h,v
> retrieving revision 1.11
> diff -u -r1.11 aparams.h
> --- aparams.h 5 Nov 2010 16:42:17 -0000 1.11
> +++ aparams.h 10 May 2011 22:58:18 -0000
> @@ -19,6 +19,8 @@
>
> #include <sys/param.h>
>
> +#include <limits.h>
> +
> #define NCHAN_MAX 16 /* max channel in a stream */
> #define RATE_MIN 4000 /* min sample rate */
> #define RATE_MAX 192000 /* max sample rate */
> @@ -64,6 +66,9 @@
>
> typedef short adata_t;
>
> +#define ADATA_MIN SHRT_MIN
> +#define ADATA_MAX SHRT_MAX
> +
these don't hurt but there's a ADATA_UNIT macro that could do the job,
I'd prefer having only one macro.
> @@ -119,6 +127,28 @@
> #else
> #error "only 16-bit and 24-bit precisions are supported"
> #endif
> +
> +/* saturating addition */
> +static inline adata_t
> +adata_sadd(register adata_t x, register adata_t y)
> +{
> +#if ADATA_BITS <= 16
> + register int sum;
> +#else
> + register long long sum;
> +#endif
> +
> + sum = x;
> + sum += y;
> +
> + if (sum < ADATA_MIN)
> + sum = ADATA_MIN;
> + else if (sum > ADATA_MAX)
> + sum = ADATA_MAX;
> +
> + return (adata_t) sum;
> +}
> +#define ADATA_SADD(x,y) adata_sadd(x,y)
>
you could inline this in mix_badd(), since it's the only place where
clipping can occur
> Index: aproc.c
> ===================================================================
> RCS file: /OpenBSD/src/usr.bin/aucat/aproc.c,v
> retrieving revision 1.64
> diff -u -r1.64 aproc.c
> --- aproc.c 28 Apr 2011 07:20:03 -0000 1.64
> +++ aproc.c 10 May 2011 22:58:19 -0000
> @@ -617,6 +617,7 @@
> unsigned i, j, cc, istart, inext, onext, ostart;
> unsigned scount, icount, ocount;
> int vol;
> + adata_t sample;
this should be 'int', adata_t is used for storage only.
>
> @@ -914,8 +916,6 @@
> struct abuf *i, *obuf = LIST_FIRST(&p->outs);
> unsigned odone;
>
> - mix_setmaster(p);
> -
this shouldn't be removed because this is the only way to avoid
clipping and keeping the full dynamic range. Perhaps make it a
device-specific option (see how "-a" option is implemented).
> Index: dev.c
> ===================================================================
> RCS file: /OpenBSD/src/usr.bin/aucat/dev.c,v
> retrieving revision 1.64
> diff -u -r1.64 dev.c
> --- dev.c 21 Oct 2010 18:57:42 -0000 1.64
> +++ dev.c 10 May 2011 22:58:19 -0000
> @@ -998,8 +998,7 @@
> }
> aproc_setin(d->mix, ibuf);
> ibuf->r.mix.xrun = xrun;
> - ibuf->r.mix.maxweight = vol;
> - mix_setmaster(d->mix);
> + ibuf->r.mix.weight = vol;
maxweight should stay there, this is the maximum volume of the
sub-device socket. It's used to cap volumes of clients of a particular
sub-device.
[...]
I don't have a clear opinion. On the one hand, clipping is not
acceptable for me. On the other hand it is statistically rare and a
lot of people here would be more annoyed by the volume step, than by
occasional clipping.
You could try fixing above issues, and let people use the diff for
long enough and see if quality is acceptable.
Perhaps disable the mix_badd() change in the beginning, since it tends
to make clipping less audible; this way we can see how often clipping
occurs.
-- Alexandre
