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

[Dorinda Bassey]

Hi Volker,

Filling a buffer with zeros to produce silence still wrong for unsigned
samples. For example, a 0 in SPA_AUDIO_FORMAT_U8 format maps to -1.0 in
SPA_AUDIO_FORMAT_F32.

This is a bug. On a buffer underrun, the buffer filled with silence is
dropped.

What are your suggestions to improve this?

Why don't you need a lock here? Is pw_stream_set_active() thread safe?

I will put a lock there, Thanks.

You only have the three volume levels 2.0, 1.0 and 0.0 while vol[i] has
256 levels.

Ack.

It's an optimization. Evaluating req = (uint64_t)v->g->dev->timer_period
* v->info.rate * 1 / 2 / 1000000 * v->frame_size once in qpw_init_out()
vs. a lot of needless evaluations every few milliseconds in the callback.

Ack

 <http://out.name> options. Please

Can you please clarify WYM here?

Thanks,

Dorinda

On Mon, Apr 3, 2023 at 8:51 AM Volker Rümelin <vr_qemu@t-online.de> wrote:

Am 28.03.23 um 13:56 schrieb Dorinda Bassey:

Hi Dorinda,

> 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().

It's an optimization. Evaluating req = (uint64_t)v->g->dev->timer_period
* v->info.rate * 1 / 2 / 1000000 * v->frame_size once in qpw_init_out()
vs. a lot of needless evaluations every few milliseconds in the callback.

With best regards,
Volker

>
>     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
>     <http://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) ""
>     >
>