On Samstag, 22. Januar 2022 13:57:31 CET Volker Rümelin wrote:
> Replace open-coded buffer arithmetic with the new function
> audio_ring_posb(). That's the position in backward direction
> of a given point at a given distance.
>
> Signed-off-by: Volker Rümelin <vr_q...@t-online.de>
> ---
First of all, getting rid of all those redundant, open coded ringbuffer
traversal code places highly makes sense!
> audio/audio.c | 25 +++++++------------------
> audio/audio_int.h | 6 ++++++
> audio/coreaudio.c | 10 ++++------
> audio/sdlaudio.c | 11 +++++------
> 4 files changed, 22 insertions(+), 30 deletions(-)
>
> diff --git a/audio/audio.c b/audio/audio.c
> index dc28685d22..e7a139e289 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -574,19 +574,13 @@ static size_t audio_pcm_sw_get_rpos_in(SWVoiceIn *sw)
> {
> HWVoiceIn *hw = sw->hw;
> ssize_t live = hw->total_samples_captured -
> sw->total_hw_samples_acquired;
> - ssize_t rpos;
>
> if (audio_bug(__func__, live < 0 || live > hw->conv_buf->size)) {
> dolog("live=%zu hw->conv_buf->size=%zu\n", live, hw->conv_buf->size);
> return 0;
> }
>
> - rpos = hw->conv_buf->pos - live;
> - if (rpos >= 0) {
> - return rpos;
> - } else {
> - return hw->conv_buf->size + rpos;
> - }
> + return audio_ring_posb(hw->conv_buf->pos, live, hw->conv_buf->size);
> }
>
> static size_t audio_pcm_sw_read(SWVoiceIn *sw, void *buf, size_t size)
> @@ -1394,12 +1388,10 @@ void audio_generic_run_buffer_in(HWVoiceIn *hw)
>
> void *audio_generic_get_buffer_in(HWVoiceIn *hw, size_t *size)
> {
> - ssize_t start = (ssize_t)hw->pos_emul - hw->pending_emul;
> + size_t start;
>
> - if (start < 0) {
> - start += hw->size_emul;
> - }
> - assert(start >= 0 && start < hw->size_emul);
> + start = audio_ring_posb(hw->pos_emul, hw->pending_emul, hw->size_emul);
> + assert(start < hw->size_emul);
>
> *size = MIN(*size, hw->pending_emul);
> *size = MIN(*size, hw->size_emul - start);
> @@ -1415,13 +1407,10 @@ void audio_generic_put_buffer_in(HWVoiceIn *hw, void
> *buf, size_t size) void audio_generic_run_buffer_out(HWVoiceOut *hw)
> {
> while (hw->pending_emul) {
> - size_t write_len, written;
> - ssize_t start = ((ssize_t) hw->pos_emul) - hw->pending_emul;
> + size_t write_len, written, start;
>
> - if (start < 0) {
> - start += hw->size_emul;
> - }
> - assert(start >= 0 && start < hw->size_emul);
> + start = audio_ring_posb(hw->pos_emul, hw->pending_emul,
> hw->size_emul);
> + assert(start < hw->size_emul);
>
> write_len = MIN(hw->pending_emul, hw->size_emul - start);
Just refactoring so far, which looks good so far.
> diff --git a/audio/audio_int.h b/audio/audio_int.h
> index 428a091d05..2fb459f34e 100644
> --- a/audio/audio_int.h
> +++ b/audio/audio_int.h
> @@ -266,6 +266,12 @@ static inline size_t audio_ring_dist(size_t dst, size_t
> src, size_t len)
> return (dst >= src) ? (dst - src) : (len - src + dst);
> }
You haven't touched this function, but while I am looking at it, all function
arguments are unsigned. So probably modulo operator might be used to get rid
of a branch here.
>
> +/* return new position in backward direction at given distance */
> +static inline size_t audio_ring_posb(size_t pos, size_t dist, size_t len)
> +{
> + return pos >= dist ? pos - dist : len - dist + pos;
> +}
> +
Which is the exact same code as the already existing audio_ring_dist()
function above, and I see that you actually already used this in v1 before:
#define audio_ring_posb(pos, dist, len) audio_ring_dist(pos, dist, len)
I would definitely not copy-paste the body. Thomas just suggested in v1 to add
a comment, not to duplicate the actual math code:
https://lore.kernel.org/qemu-devel/20220106111718.0ec25...@tuxfamily.org/
Also for consistency, I would have called the function audio_ring_rpos()
and would have commented each argument.
/**
* Returns new position in ringbuffer in backward direction at given distance.
* @pos: current position in ringbuffer
* @dist: distance in ringbuffer to walk in reverse direction
* @len: size of ringbuffer
*/
static inline size_t audio_ring_rpos(pos, dist, len) {
return audio_ring_dist(pos, dist, len);
}
At least IMO a bit more comments on math code barely hurts.
> #define dolog(fmt, ...) AUD_log(AUDIO_CAP, fmt, ## __VA_ARGS__)
>
> #ifdef DEBUG
> diff --git a/audio/coreaudio.c b/audio/coreaudio.c
> index d8a21d3e50..1fdd1d4b14 100644
> --- a/audio/coreaudio.c
> +++ b/audio/coreaudio.c
> @@ -333,12 +333,10 @@ static OSStatus audioDeviceIOProc(
>
> len = frameCount * hw->info.bytes_per_frame;
> while (len) {
> - size_t write_len;
> - ssize_t start = ((ssize_t) hw->pos_emul) - hw->pending_emul;
> - if (start < 0) {
> - start += hw->size_emul;
> - }
> - assert(start >= 0 && start < hw->size_emul);
> + size_t write_len, start;
> +
> + start = audio_ring_posb(hw->pos_emul, hw->pending_emul,
> hw->size_emul);
> + assert(start < hw->size_emul);
>
> write_len = MIN(MIN(hw->pending_emul, len),
> hw->size_emul - start);
> diff --git a/audio/sdlaudio.c b/audio/sdlaudio.c
> index c68c62a3e4..d6f3aa1a9a 100644
> --- a/audio/sdlaudio.c
> +++ b/audio/sdlaudio.c
> @@ -224,12 +224,11 @@ static void sdl_callback_out(void *opaque, Uint8 *buf,
> int len) /* dolog("callback_out: len=%d avail=%zu\n", len,
> hw->pending_emul); */
>
> while (hw->pending_emul && len) {
> - size_t write_len;
> - ssize_t start = (ssize_t)hw->pos_emul - hw->pending_emul;
> - if (start < 0) {
> - start += hw->size_emul;
> - }
> - assert(start >= 0 && start < hw->size_emul);
> + size_t write_len, start;
> +
> + start = audio_ring_posb(hw->pos_emul, hw->pending_emul,
> + hw->size_emul);
> + assert(start < hw->size_emul);
>
> write_len = MIN(MIN(hw->pending_emul, len),
> hw->size_emul - start);
This rest looks fine to me.
Best regards,
Christian Schoenebeck
