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_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.
>
> 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.0ec25...@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
