On Wed, 11 May 2011 13:07:12 +0200
Alexandre Ratchov <[email protected]> wrote:
> 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.
>
Thanks, I'll keep these in mind when making the next diff.
> [...]
>
> 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.
>
You've given me enough insight to come up with another, hopefully,
better idea. When the diff is ready I'll start a new thread.
Thanks.
> -- Alexandre
