Re: [PATCH 2/2] ALSA: pcm: auto-fill buffer with silence when draining playback

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

On Tue, Apr 11, 2023 at 04:48:58PM +0200, Jaroslav Kysela wrote:
>You're using a hammer to fix a little issue.
>
yes, but at the time it seemed like a rather small hammer to me.

if large buffers are actually a thing (what for?), then the fill could 
be limited to two periods or something. it would make the code uglier, 
though.

>Which code does not fill the last period?
>
a lot, i imagine - doing that is rather counter-intuitive when using the 
write() access method.

also, just the last period is not enough, due to the fifo, and possibly 
delayed/lost irqs.

>>> the silencing is controlled using sw_params, so applications may
>>> request the silencing before drain.
>>>
>> yeah, they could, but they don't, and most won't ever.
>> 
>> you're arguing for not doing a very practical and simple change that
>> will fix a lot of user code at once, for the sake of preventing an
>> entirely hypothetical and implausible problem. that is not a good
>> trade-off.
>
>I'm arguing that we should not do anything extra with the buffers until 
>the application requests that.

>That's the clear API context.
>
no, it's not. you cannot assume this to be understood as the central 
guiding principle which trumps more immediate issues. people use an api 
to solve a specific problem, and they want to do that with the least 
effort possible. no-one is going to read the whole docu top to bottom 
and remember every caveat. if it appears to work, people will just call 
it a day, and that's exactly what will be the case with the use of DRAIN 
(one needs a somewhat specific configuration and content to even notice 
that there is a problem).

>If we allow modification of the PCM buffer, I think that we should:
>
>- Do not modify the buffer for drivers already working with the
>   appl_ptr data (end position) only.
>
i suppose that should be detected by the drain callback being set up?

>- Handle the situation with the large buffer; it may make sense
>   to change the "wait" operation from the end-of-period interrupt to time
>   scheduler and stop the drain more early when the end-of-valid data condition
>   is fulfilled.
>
i don't understand what you're asking for.

>- Increase the protocol version.
>
>But as I wrote, I would make those extensions configurable 
>(SNDRV_PCM_HW_PARAMS_DRAIN_ALLOW_SILENCE). It can be turned on by default.
>
i have no clue what would be involved in doing that. to me that sounds 
like overkill (solving a non-issue), and goes waaaay beyond what i 
expected to invest into this issue (really, i just wanted to verify that 
the emu10k1 fixes work, and accidentally discovered that there is a 
mid-layer issue that affects user space, as the pyalsaaudio lib i'm 
using doesn't handle it).

regards