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.
+/* 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.
With best regards,
Volker
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
