Re: [PATCH v8] audio/pwaudio.c: Add Pipewire audio backend for QEMU

[Dorinda Bassey]

Hi Volker,

Thanks for the feedback.

This term is constant for the lifetime of the playback stream. It could
be precalculated in qpw_init_out().

It's still constant even when precalculated in qpw_init_out().

The if (!v->enabled) block isn't needed. When the guest stops the
playback stream, it won't write new samples. After the pipewire
ringbuffer is drained, avail is always 0. It's better to drain the
ringbuffer, otherwise the first thing you will hear after playback
starts again will be stale audio samples.

You removed the code to play silence on a buffer underrun. I suggest to
add it again. Use a trace point with the "simple" trace backend to see
how often pipewire now calls the callback in short succession for a
disabled stream before giving up. Please read again Marc-André's comments for the v7 version of the
pipewire backend. When the guest enables/disables an audio stream,
pipewire should know this. It's unnecessary that pipewire calls the
callback code for disabled streams. Don't forget to connect the stream
with the flag PW_STREAM_FLAG_INACTIVE. Every QEMU audio device enables
the stream before playback/recording starts. The pcm_ops functions volume_out and volume_in are missing. Probably
SPA_PROP_channelVolumes can be used to adjust the stream volumes.
Without these functions the guest can adjust the stream volume and the
host has an independent way to adjust the stream volume. This is
sometimes irritating.

The pipewire backend code doesn't use the in|out.name options. Please
either remove the name options or add code to connect to the specified
source/sink. I would prefer the latter. PW_KEY_TARGET_OBJECT looks
promising.

Ack.

Thanks,

Dorinda.

On Mon, Mar 20, 2023 at 7:31 AM Volker Rümelin <vr_qemu@t-online.de> wrote:

> diff --git a/audio/trace-events b/audio/trace-events
> index e1ab643add..e0acf9ac56 100644
> --- a/audio/trace-events
> +++ b/audio/trace-events
> @@ -18,6 +18,13 @@ dbus_audio_register(const char *s, const char *dir) "sender = %s, dir = %s"
>   dbus_audio_put_buffer_out(size_t len) "len = %zu"
>   dbus_audio_read(size_t len) "len = %zu"
>   
> +# pwaudio.c
> +pw_state_changed(const char *s) "stream state: %s"
> +pw_node(int nodeid) "node id: %d"
> +pw_read(int32_t avail, uint32_t index, size_t len) "avail=%u index=%u len=%zu"
> +pw_write(int32_t filled, int32_t avail, uint32_t index, size_t len) "filled=%u avail=%u index=%u len=%zu"
> +pw_audio_init(void) "Initialize Pipewire context"
> +

Hi Dorinda,

the format specifiers and parameter types don't always match.

With  best regards,
Volker

>   # audio.c
>   audio_timer_start(int interval) "interval %d ms"
>   audio_timer_stop(void) ""
>