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_q...@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_q...@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) ""
> >
>
