On Tue, Apr 11, 2023 at 12:47:26PM +0200, Jaroslav Kysela wrote:
On 05. 04. 23 22:12, Oswald Buddenhagen wrote:
This fixes a bug in thresholded mode, where we failed to use new_hw_ptr, resulting in under-fill.
I don't follow what you refer here. The old code uses snd_pcm_playback_hw_avail()
yes
thus new hw_ptr for the threshold mode, too.
not before my patch. the silencer was called before the new pointer was stored. it had to be, as otherwise the delta for top-up mode could not be calculated.
- // This will "legitimately" turn negative on underrun, and will be mangled
- // into a huge number by the boundary crossing handling. The initial state
- // might also be not quite sane. The code below MUST account for these cases.
- hw_avail = appl_ptr - runtime->status->hw_ptr;
- if (hw_avail < 0)
hw_avail += runtime->boundary;
- else if ((snd_pcm_uframes_t) hw_avail >= runtime->boundary)
hw_avail -= runtime->boundary;If hw_avail is above runtime->boundary then the initial condition is totaly bogus. I would use snd_BUG_ON() and direct return here.
this is only there as a result of inlining snd_pcm_playback_hw_avail()/snd_pcm_playback_avail() somewhat mindlessly. the check does indeed make no sense, so i'll just drop it. (the broader lesson of this is the attached patch. i can re-post it separately if you like it.)
frames = runtime->silence_threshold - noise_dist;
if ((snd_pcm_sframes_t) frames <= 0)return;The retyping does not look good here. Could we move the if before frames assignment like:
if (runtime->silence_threshold <= noise_dist) return; frames = runtime->silence_threshold - noise_dist;
dunno, i don't like it - it's more noisy and imo it loses expressiveness, as the question we're asking is "how many frames do we need to fill?". note that due to use of unsigned types in the runtime struct, such retyping is rather common in comparisons.
regards