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

[Oswald Buddenhagen <oswald.buddenhagen@gmx.de>]

On Wed, Apr 12, 2023 at 09:23:23PM +0200, Jaroslav Kysela wrote:
>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:
>>>> +	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.
>>>
>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).
>
no, that's the case where it goes negative.
because it's a subtraction, it plain cannot go over the boundary when 
both inputs are bounded.
due to the remaining correction, it could still go "very big" in an 
underrun condition, as the comment in the patch says.
we should discuss the implications of the latter for the 
snd_pcm_*_avail() functions separately (the apidoc doesn't make the 
contract clear at all).

>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.
>
that should be rather obviously done *somewhere*, as otherwise appl_ptr 
can often be even slightly above 2*boundary, at which point the above 
correction (and many alike) wouldn't even work. but for the sake of 
efficiency, that should be done asap, so when it is set or the boundary 
changes. no?

>>>>    		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?".
>
>It seems that you have answer to everything.
>
only to the parts that i actually reply to ...

>(is the requested silence threshold fulfilled? or is the noise distance 
>greater than the whole buffer / buffer_size?).
>
but why would you want to approach it that way? it's just an extra step 
to think through. to reinforce the point: if the compiler is any good, 
then your variant will be optimized into mine.

regards