audio: fix block size calculation
Currently the block size calculations are broken by design: the driver
provides a round_blocksize() function which must retrun a valid block
size in *bytes*. Unfortunately, since the driver doesn't know if it's
called for the play or for the record block size, it's mathematically
impossible to calculate the block size in all cases if play and record
number of channels are different. As a consequence, there are
half-working and weired hacks to find a usable block sizes.
The diff below addresses this by adding two new driver functions,
which are very simple to use:
set_blksz() - calculate and set the block size in *frames*, it's
necessarily common to play and recording directions no matter
the number of channels,
set_nblks() - calculate the number of blocks per buffer for the given
direction.
the diff below shows how to properly calculate the block size in
azalia and uaudio. The plan is to convert all drivers from
round_blocksize() to the new functions and to delete
round_blocksize().
Why is this important? besides for removing ugly (and risky) hacks, we
want all our drivers to support common block sizes in the 5ms-50ms
range. This would allow to implement switching between audio devices:
for instance, start playback on a USB device, unplug the cable and
continue on azalia.
OK?
Index: share/man/man9/audio.9
===
RCS file: /cvs/src/share/man/man9/audio.9,v
retrieving revision 1.27
diff -u -p -r1.27 audio.9
--- share/man/man9/audio.9 12 Mar 2019 08:18:34 - 1.27
+++ share/man/man9/audio.9 18 Aug 2019 05:28:35 -
@@ -82,6 +82,10 @@ struct audio_hw_if {
void (*)(void *), void *, struct audio_params *);
void(*copy_output)(void *hdl, size_t bytes);
void(*underrun)(void *hdl);
+ int (*set_blksz)(void *, int,
+ struct audio_params *, struct audio_params *, int);
+ int (*set_nblks)(void *, int, int,
+ struct audio_params *, int);
};
struct audio_params {
@@ -417,6 +421,56 @@ Drivers using bounce buffers for transfe
ring buffer and the device must implement this method to skip
one block from the audio ring buffer and transfer the
corresponding amount of silence to the device.
+.It Fn "int (*set_blksz)" "void *hdl" "int mode" \
+"struct audio_params *play" "struct audio_params *rec" "int blksz"
+This function is called to set the audio block size.
+.Fa mode
+is a combination of the
+.Dv AUMODE_RECORD
+and
+.Dv AUMODE_PLAY
+flags indicating the current mode set with the
+.Fn open
+function.
+The
+.Fa play
+and
+.Fa rec
+structures contain the current encoding set with the
+.Fn set_params
+function.
+.Fa blksz
+is the desired block size in frames.
+It may be adjusted to match hardware constraints.
+This function returns the adjusted block size.
+.It Fn "int (*set_nblks)" "void *hdl" "int dir" "int blksz" \
+"struct audio_params *params" "int nblks"
+This function is called to set the number of audio blocks in
+the ring buffer.
+.Fa dir
+is either the
+.Dv AUMODE_RECORD
+or the
+.Dv AUMODE_PLAY
+flag, indicating which ring buffer size is set.
+The
+.Fa params
+structure contains the encoding parameters set by the
+.Fn set_params
+method.
+.Fa blksz
+is the current block size in frames set with the
+.Fa set_params
+function.
+The
+.Fa params
+structure is the current encoding parameters, set with the
+.Fn set_params
+function.
+.Fa nblks
+is the desired number of blocks in the ring buffer.
+It may be lowered to at least two, to match hardware constraints.
+This function returns the adjusted number of blocks.
.El
.Pp
If the audio hardware is capable of input from more
Index: sys/dev/audio.c
===
RCS file: /cvs/src/sys/dev/audio.c,v
retrieving revision 1.180
diff -u -p -r1.180 audio.c
--- sys/dev/audio.c 17 Aug 2019 05:04:56 - 1.180
+++ sys/dev/audio.c 18 Aug 2019 05:28:36 -
@@ -193,6 +193,31 @@ audio_gcd(unsigned int a, unsigned int b
return a;
}
+/*
+ * Calculate the least block size (in frames) such that both the
+ * corresponding play and/or record block sizes (in bytes) are multiple
+ * of the given number of bytes.
+ */
+int
+audio_blksz_bytes(int mode,
+ struct audio_params *p, struct audio_params *r, int bytes)
+{
+ unsigned int np, nr;
+
+ if (mode & AUMODE_PLAY) {
+ np = bytes / audio_gcd(p->bps * p->channels, bytes);
+ if (!(mode & AUMODE_RECORD))
+ nr = np;
+ }
+ if (mode & AUMODE_RECORD) {
+ nr = bytes / audio_gcd(r->bps * r->channels, bytes);
+ if (!(mode & AUMODE_PLAY))
+ np = nr;
+ }
+
+ return nr * np / audio_gcd(nr, np);
+}
+
int
audio_buf_init(struct audio_softc *sc, struct audio_buf *buf, int dir)
{
@@ -625,11 +650,19 @@ audio_canstart(struct audio_
Re: audio: fix block size calculation
On 18/08/19(Sun) 08:02, Alexandre Ratchov wrote:
> Currently the block size calculations are broken by design: the driver
> provides a round_blocksize() function which must retrun a valid block
> size in *bytes*. Unfortunately, since the driver doesn't know if it's
> called for the play or for the record block size, it's mathematically
> impossible to calculate the block size in all cases if play and record
> number of channels are different. As a consequence, there are
> half-working and weired hacks to find a usable block sizes.
>
> The diff below addresses this by adding two new driver functions,
> which are very simple to use:
>
> set_blksz() - calculate and set the block size in *frames*, it's
> necessarily common to play and recording directions no matter
> the number of channels,
>
> set_nblks() - calculate the number of blocks per buffer for the given
> direction.
>
> the diff below shows how to properly calculate the block size in
> azalia and uaudio. The plan is to convert all drivers from
> round_blocksize() to the new functions and to delete
> round_blocksize().
>
> Why is this important? besides for removing ugly (and risky) hacks, we
> want all our drivers to support common block sizes in the 5ms-50ms
> range. This would allow to implement switching between audio devices:
> for instance, start playback on a USB device, unplug the cable and
> continue on azalia.
While the cleanup in itself makes sense to me. I'm unsure if continuing
to play on a secondary audio device is what we want. Nowadays phones
seems to stop music players if an audio device is disconnected.
Let's assume I'm in a hackathon hearing music via a USB device, if I
unplug it, accidentally or not, I'd find more logical that the player
stop instead of forcing the whole room to listen my music.
When gilles@ and/or jcs@ will be ready with their Bluetooth code, we
could do the same there :o)
> OK?
Diff is ok with me, if you think it makes sense to do this change anyway
:)
>
> Index: share/man/man9/audio.9
> ===
> RCS file: /cvs/src/share/man/man9/audio.9,v
> retrieving revision 1.27
> diff -u -p -r1.27 audio.9
> --- share/man/man9/audio.912 Mar 2019 08:18:34 - 1.27
> +++ share/man/man9/audio.918 Aug 2019 05:28:35 -
> @@ -82,6 +82,10 @@ struct audio_hw_if {
> void (*)(void *), void *, struct audio_params *);
> void(*copy_output)(void *hdl, size_t bytes);
> void(*underrun)(void *hdl);
> + int (*set_blksz)(void *, int,
> + struct audio_params *, struct audio_params *, int);
> + int (*set_nblks)(void *, int, int,
> + struct audio_params *, int);
> };
>
> struct audio_params {
> @@ -417,6 +421,56 @@ Drivers using bounce buffers for transfe
> ring buffer and the device must implement this method to skip
> one block from the audio ring buffer and transfer the
> corresponding amount of silence to the device.
> +.It Fn "int (*set_blksz)" "void *hdl" "int mode" \
> +"struct audio_params *play" "struct audio_params *rec" "int blksz"
> +This function is called to set the audio block size.
> +.Fa mode
> +is a combination of the
> +.Dv AUMODE_RECORD
> +and
> +.Dv AUMODE_PLAY
> +flags indicating the current mode set with the
> +.Fn open
> +function.
> +The
> +.Fa play
> +and
> +.Fa rec
> +structures contain the current encoding set with the
> +.Fn set_params
> +function.
> +.Fa blksz
> +is the desired block size in frames.
> +It may be adjusted to match hardware constraints.
> +This function returns the adjusted block size.
> +.It Fn "int (*set_nblks)" "void *hdl" "int dir" "int blksz" \
> +"struct audio_params *params" "int nblks"
> +This function is called to set the number of audio blocks in
> +the ring buffer.
> +.Fa dir
> +is either the
> +.Dv AUMODE_RECORD
> +or the
> +.Dv AUMODE_PLAY
> +flag, indicating which ring buffer size is set.
> +The
> +.Fa params
> +structure contains the encoding parameters set by the
> +.Fn set_params
> +method.
> +.Fa blksz
> +is the current block size in frames set with the
> +.Fa set_params
> +function.
> +The
> +.Fa params
> +structure is the current encoding parameters, set with the
> +.Fn set_params
> +function.
> +.Fa nblks
> +is the desired number of blocks in the ring buffer.
> +It may be lowered to at least two, to match hardware constraints.
> +This function returns the adjusted number of blocks.
> .El
> .Pp
> If the audio hardware is capable of input from more
> Index: sys/dev/audio.c
> ===
> RCS file: /cvs/src/sys/dev/audio.c,v
> retrieving revision 1.180
> diff -u -p -r1.180 audio.c
> --- sys/dev/audio.c 17 Aug 2019 05:04:56 - 1.180
> +++ sys/dev/audio.c 18 Aug 2019 05:28:36 -
> @@ -193,6 +193,31 @@ audio_gcd(unsigned int a, unsigned int b
> return a;
> }
>
> +/*
> + * Calculate the least block size (in
Re: audio: fix block size calculation
On Tue, Sep 03, 2019 at 02:03:52PM -0300, Martin Pieuchot wrote: > On 18/08/19(Sun) 08:02, Alexandre Ratchov wrote: > > Currently the block size calculations are broken by design: the driver > > provides a round_blocksize() function which must retrun a valid block > > size in *bytes*. Unfortunately, since the driver doesn't know if it's > > called for the play or for the record block size, it's mathematically > > impossible to calculate the block size in all cases if play and record > > number of channels are different. As a consequence, there are > > half-working and weired hacks to find a usable block sizes. > > > > The diff below addresses this by adding two new driver functions, > > which are very simple to use: > > > > set_blksz() - calculate and set the block size in *frames*, it's > > necessarily common to play and recording directions no matter > > the number of channels, > > > > set_nblks() - calculate the number of blocks per buffer for the given > > direction. > > > > the diff below shows how to properly calculate the block size in > > azalia and uaudio. The plan is to convert all drivers from > > round_blocksize() to the new functions and to delete > > round_blocksize(). > > > > Why is this important? besides for removing ugly (and risky) hacks, we > > want all our drivers to support common block sizes in the 5ms-50ms > > range. This would allow to implement switching between audio devices: > > for instance, start playback on a USB device, unplug the cable and > > continue on azalia. > > While the cleanup in itself makes sense to me. I'm unsure if continuing > to play on a secondary audio device is what we want. Nowadays phones > seems to stop music players if an audio device is disconnected. > > Let's assume I'm in a hackathon hearing music via a USB device, if I > unplug it, accidentally or not, I'd find more logical that the player > stop instead of forcing the whole room to listen my music. I totally agree, making this the default would be a mistake as it's against the "least surprise" principle. It seems practical in few specific cases though: for instance I use it to temporarilly connect a usb headset in order to answer a phone call in firefox without the need to restart sndiod and firefox and then call back the other person. > > OK? > > Diff is ok with me, if you think it makes sense to do this change anyway > :) > Sure, the change is very useful.
