| Message ID | 20230105153546.468288-1-consult.awy@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pcm_rate: fix hw_ptr boundary wrapping issue | expand |
Hi, did this one slip past? I have not see any response. Alan. -------- Forwarded Message -------- Subject: [PATCH] pcm_rate: fix hw_ptr boundary wrapping issue Date: Thu, 5 Jan 2023 15:35:46 +0000 From: Alan Young [1]<consult.awy@gmail.com> To: [2]alsa-devel@alsa-project.org CC: [3]tiwai@suse.de, [4]mahendran.kuppusamy@in.bosch.com, Alan Young [5]<consult.awy@gmail.com> Wrap the hw_ptr using the total position of the slave hw_ptr, including boundary wraps. Otherwise, small errors can creep in due to residuals (when boundary is not a multiple of period size) and which can accumulate. Signed-off-by: Alan Young [6]<consult.awy@gmail.com> Fixes: 7570e5d7 ("pcm: rate: fix the hw_ptr update until the boundary available") --- src/pcm/pcm_rate.c | 54 ++++++++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 18 deletions(-) diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c index c8076859..a29fc5a9 100644 --- a/src/pcm/pcm_rate.c +++ b/src/pcm/pcm_rate.c @@ -52,6 +52,7 @@ struct _snd_pcm_rate { snd_pcm_generic_t gen; snd_pcm_uframes_t appl_ptr, hw_ptr, last_slave_hw_ptr; snd_pcm_uframes_t last_commit_ptr; + u_int64_t slave_hw_ptr_wrap; snd_pcm_uframes_t orig_avail_min; snd_pcm_sw_params_t sw_params; snd_pcm_format_t sformat; @@ -562,6 +563,8 @@ static int snd_pcm_rate_init(snd_pcm_t *pcm) if (rate->ops.reset) rate->ops.reset(rate->obj); rate->last_commit_ptr = 0; + rate->last_slave_hw_ptr = 0; + rate->slave_hw_ptr_wrap = 0; rate->start_pending = 0; return 0; } @@ -638,33 +641,48 @@ snd_pcm_rate_read_areas1(snd_pcm_t *pcm, static inline void snd_pcm_rate_sync_hwptr0(snd_pcm_t *pcm, snd_pcm_uframes_t slave_hw_ptr) { snd_pcm_rate_t *rate; - snd_pcm_sframes_t slave_hw_ptr_diff; - snd_pcm_sframes_t last_slave_hw_ptr_frac; + snd_pcm_uframes_t new_hw_ptr; + snd_pcm_uframes_t slave_residual; if (pcm->stream != SND_PCM_STREAM_PLAYBACK) return; rate = pcm->private_data; - slave_hw_ptr_diff = pcm_frame_diff(slave_hw_ptr, rate->last_slave_hw_ptr, rate->gen.slave->boundary); - if (slave_hw_ptr_diff == 0) + if (slave_hw_ptr == rate->last_slave_hw_ptr) return; - last_slave_hw_ptr_frac = rate->last_slave_hw_ptr % rate->gen.slave->period_size; - /* While handling fraction part fo slave period, rounded value will be - * introduced by input_frames(). - * To eliminate rounding issue on rate->hw_ptr, subtract last rounded - * value from rate->hw_ptr and add new rounded value of present - * slave_hw_ptr fraction part to rate->hw_ptr. Hence, - * rate->hw_ptr += [ (no. of updated slave periods * pcm rate period size) - - * fractional part of last_slave_hw_ptr rounded value + - * fractional part of updated slave hw ptr's rounded value ] + + /* + * Our hw_ptr may wrap at a different time to that of the slave as the + * number of period / boundary may differ. It is essential that our hw_ptr + * wraps at the correct point, along with our appl_ptr. Otherwise the + * avail calculation will be incorrect. + * + * Track the number of slave hw_ptr boundary wraps and use that to calculate + * our new_hw_ptr from the total number of samples converted. */ - rate->hw_ptr += ( - (((last_slave_hw_ptr_frac + slave_hw_ptr_diff) / rate->gen.slave->period_size) * pcm->period_size) - - rate->ops.input_frames(rate->obj, last_slave_hw_ptr_frac) + - rate->ops.input_frames(rate->obj, (last_slave_hw_ptr_frac + slave_hw_ptr_diff) % rate->gen.slave->period_size)); + if (slave_hw_ptr < rate->last_slave_hw_ptr) { + rate->slave_hw_ptr_wrap += rate->gen.slave->boundary; + } rate->last_slave_hw_ptr = slave_hw_ptr; - rate->hw_ptr %= pcm->boundary; + /* + * Calculate rate->hw_ptr using total number of frames consumed by slave hw_ptr, + * including any boundary wraps. + */ + if (rate->slave_hw_ptr_wrap) { + /* + * Restrict explicit 64-bit calculations to case where rate->slave_hw_ptr_wrap + * is non-zero, which will only happen in 32-bit environments. + */ + u_int64_t wrapped_slave_hw_ptr = slave_hw_ptr + rate->slave_hw_ptr_wrap; + new_hw_ptr = ((wrapped_slave_hw_ptr / rate->gen.slave->period_size) * pcm->period_size) % pcm->boundary; + slave_residual = wrapped_slave_hw_ptr % rate->gen.slave->period_size; + } else { + new_hw_ptr = (slave_hw_ptr / rate->gen.slave->period_size) * pcm->period_size; + slave_residual = slave_hw_ptr % rate->gen.slave->period_size; + } + new_hw_ptr += rate->ops.input_frames(rate->obj, slave_residual); + rate->hw_ptr = new_hw_ptr % pcm->boundary; } static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm) -- 2.25.1 References 1. mailto:consult.awy@gmail.com 2. mailto:alsa-devel@alsa-project.org 3. mailto:tiwai@suse.de 4. mailto:mahendran.kuppusamy@in.bosch.com 5. mailto:consult.awy@gmail.com 6. mailto:consult.awy@gmail.com
On 05. 01. 23 16:35, Alan Young wrote: > Wrap the hw_ptr using the total position of the slave hw_ptr, including > boundary wraps. Otherwise, small errors can creep in due to residuals > (when boundary is not a multiple of period size) and which can > accumulate. > > Signed-off-by: Alan Young <consult.awy@gmail.com> > Fixes: 7570e5d7 ("pcm: rate: fix the hw_ptr update until the boundary available") > --- > src/pcm/pcm_rate.c | 54 ++++++++++++++++++++++++++++++---------------- > 1 file changed, 36 insertions(+), 18 deletions(-) > > diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c > index c8076859..a29fc5a9 100644 > --- a/src/pcm/pcm_rate.c > +++ b/src/pcm/pcm_rate.c ... > + if (rate->slave_hw_ptr_wrap) { > + /* > + * Restrict explicit 64-bit calculations to case where rate->slave_hw_ptr_wrap > + * is non-zero, which will only happen in 32-bit environments. > + */ > + u_int64_t wrapped_slave_hw_ptr = slave_hw_ptr + rate->slave_hw_ptr_wrap; > + new_hw_ptr = ((wrapped_slave_hw_ptr / rate->gen.slave->period_size) * pcm->period_size) % pcm->boundary; > + slave_residual = wrapped_slave_hw_ptr % rate->gen.slave->period_size; I don't think that this calculation is correct. If the boundary differs by more than buffer_size, the new_hw_ptr will be cropped (downsampling). It will be probably much better to track only pointer diffs and let hw/appl ptrs updating independently like we do in other plugins. Jaroslav
On 16. 02. 23 14:45, Alan Young wrote: > Hi, did this one slip past? I have not see any response. Alan. I replied to the original e-mail. Usually, it's better to continue in a thread than to do a malformed forward. Jaroslav > > -------- Forwarded Message -------- > Subject: [PATCH] pcm_rate: fix hw_ptr boundary wrapping issue > Date: Thu, 5 Jan 2023 15:35:46 +0000 > From: Alan Young [1]<consult.awy@gmail.com> > To: [2]alsa-devel@alsa-project.org > CC: [3]tiwai@suse.de, [4]mahendran.kuppusamy@in.bosch.com, Alan Young > [5]<consult.awy@gmail.com> > Wrap the hw_ptr using the total position of the slave hw_ptr, including > boundary wraps. Otherwise, small errors can creep in due to residuals > (when boundary is not a multiple of period size) and which can > accumulate. > Signed-off-by: Alan Young [6]<consult.awy@gmail.com> > Fixes: 7570e5d7 ("pcm: rate: fix the hw_ptr update until the boundary > available") > --- > src/pcm/pcm_rate.c | 54 ++++++++++++++++++++++++++++++---------------- > 1 file changed, 36 insertions(+), 18 deletions(-) > diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c ...
diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c index c8076859..a29fc5a9 100644 --- a/src/pcm/pcm_rate.c +++ b/src/pcm/pcm_rate.c @@ -52,6 +52,7 @@ struct _snd_pcm_rate { snd_pcm_generic_t gen; snd_pcm_uframes_t appl_ptr, hw_ptr, last_slave_hw_ptr; snd_pcm_uframes_t last_commit_ptr; + u_int64_t slave_hw_ptr_wrap; snd_pcm_uframes_t orig_avail_min; snd_pcm_sw_params_t sw_params; snd_pcm_format_t sformat; @@ -562,6 +563,8 @@ static int snd_pcm_rate_init(snd_pcm_t *pcm) if (rate->ops.reset) rate->ops.reset(rate->obj); rate->last_commit_ptr = 0; + rate->last_slave_hw_ptr = 0; + rate->slave_hw_ptr_wrap = 0; rate->start_pending = 0; return 0; } @@ -638,33 +641,48 @@ snd_pcm_rate_read_areas1(snd_pcm_t *pcm, static inline void snd_pcm_rate_sync_hwptr0(snd_pcm_t *pcm, snd_pcm_uframes_t slave_hw_ptr) { snd_pcm_rate_t *rate; - snd_pcm_sframes_t slave_hw_ptr_diff; - snd_pcm_sframes_t last_slave_hw_ptr_frac; + snd_pcm_uframes_t new_hw_ptr; + snd_pcm_uframes_t slave_residual; if (pcm->stream != SND_PCM_STREAM_PLAYBACK) return; rate = pcm->private_data; - slave_hw_ptr_diff = pcm_frame_diff(slave_hw_ptr, rate->last_slave_hw_ptr, rate->gen.slave->boundary); - if (slave_hw_ptr_diff == 0) + if (slave_hw_ptr == rate->last_slave_hw_ptr) return; - last_slave_hw_ptr_frac = rate->last_slave_hw_ptr % rate->gen.slave->period_size; - /* While handling fraction part fo slave period, rounded value will be - * introduced by input_frames(). - * To eliminate rounding issue on rate->hw_ptr, subtract last rounded - * value from rate->hw_ptr and add new rounded value of present - * slave_hw_ptr fraction part to rate->hw_ptr. Hence, - * rate->hw_ptr += [ (no. of updated slave periods * pcm rate period size) - - * fractional part of last_slave_hw_ptr rounded value + - * fractional part of updated slave hw ptr's rounded value ] + + /* + * Our hw_ptr may wrap at a different time to that of the slave as the + * number of period / boundary may differ. It is essential that our hw_ptr + * wraps at the correct point, along with our appl_ptr. Otherwise the + * avail calculation will be incorrect. + * + * Track the number of slave hw_ptr boundary wraps and use that to calculate + * our new_hw_ptr from the total number of samples converted. */ - rate->hw_ptr += ( - (((last_slave_hw_ptr_frac + slave_hw_ptr_diff) / rate->gen.slave->period_size) * pcm->period_size) - - rate->ops.input_frames(rate->obj, last_slave_hw_ptr_frac) + - rate->ops.input_frames(rate->obj, (last_slave_hw_ptr_frac + slave_hw_ptr_diff) % rate->gen.slave->period_size)); + if (slave_hw_ptr < rate->last_slave_hw_ptr) { + rate->slave_hw_ptr_wrap += rate->gen.slave->boundary; + } rate->last_slave_hw_ptr = slave_hw_ptr; - rate->hw_ptr %= pcm->boundary; + /* + * Calculate rate->hw_ptr using total number of frames consumed by slave hw_ptr, + * including any boundary wraps. + */ + if (rate->slave_hw_ptr_wrap) { + /* + * Restrict explicit 64-bit calculations to case where rate->slave_hw_ptr_wrap + * is non-zero, which will only happen in 32-bit environments. + */ + u_int64_t wrapped_slave_hw_ptr = slave_hw_ptr + rate->slave_hw_ptr_wrap; + new_hw_ptr = ((wrapped_slave_hw_ptr / rate->gen.slave->period_size) * pcm->period_size) % pcm->boundary; + slave_residual = wrapped_slave_hw_ptr % rate->gen.slave->period_size; + } else { + new_hw_ptr = (slave_hw_ptr / rate->gen.slave->period_size) * pcm->period_size; + slave_residual = slave_hw_ptr % rate->gen.slave->period_size; + } + new_hw_ptr += rate->ops.input_frames(rate->obj, slave_residual); + rate->hw_ptr = new_hw_ptr % pcm->boundary; } static inline void snd_pcm_rate_sync_hwptr(snd_pcm_t *pcm)
Wrap the hw_ptr using the total position of the slave hw_ptr, including boundary wraps. Otherwise, small errors can creep in due to residuals (when boundary is not a multiple of period size) and which can accumulate. Signed-off-by: Alan Young <consult.awy@gmail.com> Fixes: 7570e5d7 ("pcm: rate: fix the hw_ptr update until the boundary available") --- src/pcm/pcm_rate.c | 54 ++++++++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 18 deletions(-)