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

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Dienstag, 25. Januar 2022 19:57:59 CET Volker Rümelin wrote:
> > 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.
> 
> That would be "return (len - dist + pos) % len;" but on my x86_64 system
> I always prefer a conditional move instruction to a 64 bit integer
> division instruction.

Nevermind, I just mentioned it as a side note, here in QEMU it probably 
doesn't matter at all. In other apps this is sometimes used in tight loops 
where it can make a measurable difference to get rid of the branch(es).

> 
> >> +/* 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.
> 
> In the audio subsystem rpos is typically the read position. I chose posb
> to distinguish it from read position.
> 
> > /**
> > 
> >   * 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
> >   */
> 
> This comment is better than my comment. I'll use it in my v3 series.
> 
> > static inline size_t audio_ring_rpos(pos, dist, len) {
> > 
> >      return audio_ring_dist(pos, dist, len);
> > 
> > }
> 
> I don't think this inline function improves readability compared to my
> macro from v1. To understand the code you still have to replace
> parameter names in your mind. My v2 inline function can be understood at
> first glance.

I didn't mean readability. Believe it or not, it took me a bit to realize that 
it was the exact same code as above.

Best regards,
Christian Schoenebeck