Re: [PATCH 1/2] ALSA: pcm: rewrite snd_pcm_playback_silence()

[Jaroslav Kysela <perex@perex.cz>]

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@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.