Re: [PATCH] alsaaudio: Set try-poll to false by default

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: text/plain, Size: 9580 bytes --]

On Wed, 7 May 2025, BALATON Zoltan wrote:
> On Wed, 7 May 2025, Christian Schoenebeck wrote:
>> On Tuesday, May 6, 2025 4:34:42 PM CEST BALATON Zoltan wrote:
>>> On Tue, 8 Apr 2025, Volker Rümelin wrote:
>>>> Am 08.04.25 um 14:55 schrieb Christian Schoenebeck:
>>>>> On Friday, April 4, 2025 1:34:27 PM CEST BALATON Zoltan wrote:
>>>>>> On Fri, 4 Apr 2025, Christian Schoenebeck wrote:
>>>>>>> On Monday, March 31, 2025 3:05:24 PM CEST BALATON Zoltan wrote:
>>>>>>>> On Sun, 23 Mar 2025, Christian Schoenebeck wrote:
>>>>>>>>> On Sunday, March 16, 2025 1:20:46 AM CET BALATON Zoltan wrote:
>>>>>>>>>> Quoting Volker Rümelin: "try-poll=on tells the ALSA backend to try 
>>>>>>>>>> to
>>>>>>>>>> use an event loop instead of the audio timer. This works most of 
>>>>>>>>>> the
>>>>>>>>>> time. But the poll event handler in the ALSA backend has a bug. For
>>>>>>>>>> example, if the guest can't provide enough audio frames in time, 
>>>>>>>>>> the
>>>>>>>>>> ALSA buffer is only partly full and the event handler will be 
>>>>>>>>>> called
>>>>>>>>>> again and again on every iteration of the main loop. This increases
>>>>>>>>>> the processor load and the guest has less processor time to provide
>>>>>>>>>> new audio frames in time. I have two examples where a guest can't
>>>>>>>>>> recover from this situation and the guest seems to hang."
>>>>>>>>>> 
>>>>>>>>>> One reproducer I've found is booting MorphOS demo iso on
>>>>>>>>>> qemu-system-ppc -machine pegasos2 -audio alsa which should play a
>>>>>>>>>> startup sound but instead it freezes. Even when it does not hang it
>>>>>>>>>> plays choppy sound. Volker suggested using command line to set
>>>>>>>>>> try-poll=off saying: "The try-poll=off arguments are typically
>>>>>>>>>> necessary, because the alsa backend has a design issue with
>>>>>>>>>> try-poll=on. If the guest can't provide enough audio frames, it's
>>>>>>>>>> really unhelpful to ask for new audio frames on every main loop
>>>>>>>>>> iteration until the guest can provide enough audio frames. Timer 
>>>>>>>>>> based
>>>>>>>>>> playback doesn't have that problem."
>>>>>>>>>> 
>>>>>>>>>> But users cannot easily find this option and having a non-working
>>>>>>>>>> default is really unhelpful so to make life easier just set it to
>>>>>>>>>> false by default which works until the issue with the alsa backend 
>>>>>>>>>> can
>>>>>>>>>> be fixed.
>>>>>>>>>> 
>>>>>>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>>>>>>> ---
>>>>>>>>>> This fixes my issue but if somebody has a better fix I'm open to 
>>>>>>>>>> that
>>>>>>>>>> too.
>>>>>>>>>>
>>>>>>>>>>  audio/alsaaudio.c | 2 +-
>>>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>> 
>>>>>>>>>> diff --git a/audio/alsaaudio.c b/audio/alsaaudio.c
>>>>>>>>>> index cacae1ea59..9b6c01c0ef 100644
>>>>>>>>>> --- a/audio/alsaaudio.c
>>>>>>>>>> +++ b/audio/alsaaudio.c
>>>>>>>>>> @@ -899,7 +899,7 @@ static void alsa_enable_in(HWVoiceIn *hw, bool 
>>>>>>>>>> enable)
>>>>>>>>>>  static void 
>>>>>>>>>> alsa_init_per_direction(AudiodevAlsaPerDirectionOptions *apdo)
>>>>>>>>>>  {
>>>>>>>>>>      if (!apdo->has_try_poll) {
>>>>>>>>>> -        apdo->try_poll = true;
>>>>>>>>>> +        apdo->try_poll = false;
>>>>>>>>>>          apdo->has_try_poll = true;
>>>>>>>>>>      }
>>>>>>>>>>  }
>>>>>>>>>> 
>>>>>>>>> Correct me if I am wrong, but AFAICS if polling is not used then no 
>>>>>>>>> state
>>>>>>>>> changes would be handled, no? At least I don't see any 
>>>>>>>>> snd_pcm_state() call
>>>>>>>>> outside of alsa_poll_handler().
>>>>>>>> I have no idea but this fixes the problem (and does the same that can 
>>>>>>>> be
>>>>>>>> also done from command line but nobody can find that command line 
>>>>>>>> option)
>>>>>>>> so unless somebody has a better idea could this be merged as a fix 
>>>>>>>> for
>>>>>>>> now?
>>>>>>> Well, I understand that if fixes the misbehaviour you encountered. But 
>>>>>>> how
>>>>>>> helpful would it be if it then breaks behaviour for other people 
>>>>>>> instead?
>>>>>> What behaviour would it break and how?
>>>>> There are only a bunch of ALSA states handled right now in the QEMU Alsa
>>>>> driver (see alsa_poll_handler()):
>>>>>
>>>>>     state = snd_pcm_state (hlp->handle);
>>>>>     switch (state) {
>>>>>     case SND_PCM_STATE_SETUP:
>>>>>         alsa_recover (hlp->handle);
>>>>>         break;
>>>>>
>>>>>     case SND_PCM_STATE_XRUN:
>>>>>         alsa_recover (hlp->handle);
>>>>>         break;
>>>>>
>>>>>     case SND_PCM_STATE_SUSPENDED:
>>>>>         alsa_resume (hlp->handle);
>>>>>         break;
>>>>>
>>>>>     case SND_PCM_STATE_PREPARED:
>>>>>         audio_run(hlp->s, "alsa run (prepared)");
>>>>>         break;
>>>>>
>>>>>     case SND_PCM_STATE_RUNNING:
>>>>>         audio_run(hlp->s, "alsa run (running)");
>>>>>         break;
>>>>> 
>>>>> For instance in poll mode it recovers in case of an xrun, which happens 
>>>>> on
>>>>> audio output if the audio output data was not delivered by the 
>>>>> application in
>>>>> time.
>>>>> 
>>>>> The other case is when the system was suspended (standby). It should 
>>>>> also
>>>>> recover the audio session here.
>>>> 
>>>> Hi Christian,
>>>> 
>>>> I think the timer based mode works fine. snd_pcm_readi() and
>>>> snd_pcm_writei() return -EPIPE in case of a xrun and -ESTRPIPE if a
>>>> suspend event occurred. Both cases are handled in alsa_write().
>> 
>> Yeah, I think you are right Volker. The already existing -EPIPE and 
>> -ESTRPIPE
>> error cases should handle the "xrun" and "suspended" conditions 
>> equivalently
>> in poll mode.
>> 
>>>> The -ESTRPIPE case is missing in alsa_read(), which may be a mistake. I
>>>> don't think it's possible alsa_read() and alsa_write() get called if the
>>>> ALSA system is in state SND_PCM_STATE_SETUP.
>> 
>> Agreed, -ESTRPIPE is missing in alsa_read(), and -EBADF (audio device in
>> invalid state) is missing in both alsa_read() and alsa_write(). That could
>> probably happen when you unplug an USB audio device. On macOS applications
>> also need to recover when plugging in/out headphones, but IIRC that's not
>> necessary with ALSA.
>> 
>>>> The write_loop() example code at
>>>> https://www.alsa-project.org/alsa-doc/alsa-lib/examples.html in file
>>>> test/pcm.c also doesn't use the snd_pcm_state() function. Please have a
>>>> look at write_loop() in test/pcm.c and compare it with
>>>> write_and_poll_loop() in the same file.
>>>> 
>>>> With best regards
>>>> Volker
>>> 
>>> Could we get back to this and either accept this patch or find another
>>> solution? From the above, it looks like this could be an acceptable fix
>>> unless we can prove it would break something (but then it's already
>>> possible to break it from the command line option so it's preexisting
>>> problem).
>> 
>> Well, I wouldn't say it's exactly the same thing to experience a 
>> misbehaviour
>> with a specific, custom supplied option vs. experiencing the misbehaviour 
>> when
>> running with default options. And I am pretty sure you know why. ;-)
>
> Considering that most people use distros that have pulsaudio or pipewire or 
> some other sound server they likely already need a command line option to 
> enable alsa but needing an additional obscure option to make that work that's 
> not really documented at any easy to find place makes that much worse. So a 
> mostly working default is still better than a surely broken one.
>
>> But anyway, I won't object this change:
>> 
>> Acked-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
>
> Thank you, hope some maintainer sees this and pick up this patch.

If there will be a pull request with Volker's fixes could this be also 
included? Link:

https://patchew.org/QEMU/20250316002046.D066A4E6004@zero.eik.bme.hu/

Regards,
BALATON Zoltan

>>>>> Now I haven't tested whether these would work in callback mode right 
>>>>> now, but
>>>>> looking at the code suggests that they might not.
>>>>> 
>>>>>>> I think it would be better to add a 2nd patch that would handle state 
>>>>>>> changes
>>>>>>> in callback mode. That would satisfy both groups of people. AFAICS
>>>>>>> snd_pcm_state() can be called both in polling mode and callback mode.
>>>>>> I can't do that because I don't quite know neither alsa nor audio in 
>>>>>> QEMU
>>>>>> so I have no idea what to do. Can you give more clues?
>>>>> Well, as a starting point you might try whether these cases described 
>>>>> above
>>>>> would still work in callback mode. Maybe it is even working, who knows.
>>> 
>>> Can you suggest how can I test that? I'm not sure what to try and sound
>>> works with this patch for the cases I use.
>> 
>> A suspend condition is simply putting the host machine asleep, i.e. closing
>> the lid of your Linux notebook, or selecting suspend from the menu of your
>> desktop environment.
>
> I have a desktop machine that I don't normally suspend and the menu item does 
> not seem to work so I can't test that. Is just stopping QEMU process enough 
> or the host has to be restarted while QEMU is running to test this?
>
>> For testing for xruns (a.k.a. buffer overrun / buffer underrun) you could
>> simply insert a short sleep in the render code to simulate the audio render
>> function to take too long to deliver the audio data to the ALSA subsystem.
>
> I don't know audio in QEMU so don't know where to insert such sleep. Maybe I 
> could test this if you can give more specific advice.
>
> Regards,
> BALATON Zoltan
>
>> In both cases QEMU should recover audio. If it doesn't, you will simply
>> continue to hear silence with QEMU for good.
>> 
>> /Christian
>> 
>> 
>