Re: [PATCH v2 01/15] audio: replace open-coded buffer arithmetic

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

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_qemu@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.0ec25383@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