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

[Volker Rümelin]

Hi Dorinda,

> 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?

The code in patch v7 handled buffer underruns in playback_on_process() 
correctly. I suggest to use that part of the code again. It was just 
wrong to fill the buffer with zeros for unsigned samples. Christian 
suggested to use the audio_pcm_info_clear_buf() function instead of 
memset(p, 0, n_bytes). If you don't want to use 
audio_pcm_info_clear_buf() you could use the code there as a template.

There is no guarantee that guests can produce audio samples fast enough. 
Buffer underruns should therefore be handled properly.

>     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?

I didn't write that. The link was already in your email.

With best regards,
Volker

> 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>
>     >     <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) ""
>     >     >
>     >