On 12. 04. 23 12:33, Oswald Buddenhagen wrote:
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.)
I will correct that it will make sense where hw_ptr is nearby boundary
(boundary - buffer_size ... boundary - 1) and appl_ptr is cropped using
boundary (0 ... buffer_size). But because appl_ptr can be set by application
without any kernel side correction, it may be possible to check if the
appl_ptr is in 0 ... boundary range before any use. Sorry for the confusion.
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.
It seems that you have answer to everything. My suggestion is perfectly
readable (is the requested silence threshold fulfilled? or is the noise
distance greater than the whole buffer / buffer_size?).
Jaroslav
--
Jaroslav Kysela <perex@xxxxxxxx>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
