Pipewire agent #157

DemiMarie · 2022-04-29T17:07:45Z

This now actually works.

Edit: This builds ordinary Fedora/Debian/Arch packages. To use them, install them in your VM of choice, then enable PipeWire with qvm-service somevm pipewire on.

Edit 2: I have fixed all the bugs that I am aware of and able to find. There is still one remaining bug: if I am recording an audio stream, and start playback while the recording is in progress, audio playback may stop working. However, I believe this is not an agent bug, but rather a bug in pacat-simple-vchan, as I am also able to get pacat-simple-vchan to behave strangely in other ways (such as not responding to control requests while recording silence). Now fixed I think.

Edit 3: All remaining bugs appear to be fixed, at least if pacat-simple-vchan has QubesOS/qubes-gui-daemon#109.

Edit 4: Remaining to-do items (should not block merge):

Allow some properties (such as buffer sizes) properties to be configured via QubesDB as well as the configuration file.
Watch /qubes-audio-domain-xid for changes. If it does change, disconnect from the old audio VM and connect to the new one.
Provide an interface that an audio VM can use to differentiate between a microphone being muted and disconnected. Useful for workloads that care about this distinction, if any.
Allow the node name to be configured via the configuration file.
Integration with qubes-video-companion for video support.
Implement buffer negotiation as suggested in the header file.

marmarek · 2022-05-04T18:48:33Z

PipelineRefresh

DemiMarie · 2022-06-27T03:44:08Z

PipelineRetry

DemiMarie · 2022-07-01T23:34:22Z

PipelineRefresh

DemiMarie · 2022-07-02T17:16:51Z

PipelineRetry

pipewire/GNUmakefile

DemiMarie · 2022-07-05T05:39:49Z

At this point, the only code that I know to be missing is the Debian and Arch maintainer scripts to manage PipeWire-related systemd units.

pipewire/GNUmakefile

pipewire/qubes-pw-module.c

marmarek · 2022-07-28T02:35:43Z

pipewire/qubes-pw-module.c

+        break;
+    case SPA_PARAM_Props:;
+        _Static_assert(SPA_PARAM_Props == 2, "wrong SPA_PARAM_Props");
+        /* TODO: reconfigure the stream according to the new properties */


Is there anything that is actually worth handling (and is possible to do)? If so, maybe log a message for now? Otherwise, drop the comment or at least its "TODO" part.

I honestly do not know, hence the TODO.

pipewire/qubes-pw-module.c

DemiMarie

Comments (hopefully) addressed. I will not be able to re-test until tomorrow.

pipewire/qubes-pw-module.c

DemiMarie · 2022-07-28T05:28:38Z

pipewire/qubes-pw-module.c

+        break;
+    case SPA_PARAM_Props:;
+        _Static_assert(SPA_PARAM_Props == 2, "wrong SPA_PARAM_Props");
+        /* TODO: reconfigure the stream according to the new properties */


I honestly do not know, hence the TODO.

pipewire/qubes-pw-module.c

marmarek

Some packaging comments

debian/control

rpm_spec/gui-agent.spec.in

debian/qubes-vm-pipewire.postinst

pipewire/qubes-pw-module.c

marmarek · 2022-07-31T16:51:37Z

Some quick test results (this PR + stock pacat-simple-vchan):

playback seems to work, both via pipewire natively and via pipewire-pulseaudio
recording seems to work with pipewire natively, but is very choppy via pipewire-pulseaudio
the module seems to properly sent record start/stop commands
the module seems to not send playback start/stop commands, instead it sends silence (zeroes) when nothing is playing -> this prevents the actual device to suspend when unused, and results in constant CPU usage on the pacat-simple-vchan side

marmarek · 2022-07-31T23:28:38Z

when pipewire is started first, then some client (trying to) play something and only then pacat-simple-vchan, then log has endless stream of Overrun: asled tp wrote 2048 bytes, but can only write 0
when recording is started in VM first, but recording is allowed later, there is a stream of Underrun: asked to read 2048 bytes, but only xx available (where xx is mostly non-zero, but sometimes 0). At ~~one case~~ two cases, when I stopped recording in the VM (closed the client), then pipewire crashed with SIGABRT and corrupted size vs prev_size message (I have a backtrace and coredump, but I'm not sure how useful they will be in practice)

In both of the above, the client is really connected via pipewire-pulseaudio (pavucontrol and/or firefox playing something)

DemiMarie · 2022-08-01T08:37:55Z

Some quick test results (this PR + stock pacat-simple-vchan):

recording seems to work with pipewire natively, but is very choppy via pipewire-pulseaudio

Will need to debug this. Does changing the max quantum in 30_qubes.conf help?

the module seems to not send playback start/stop commands, instead it sends silence (zeroes) when nothing is playing -> this prevents the actual device to suspend when unused, and results in constant CPU usage on the pacat-simple-vchan side

I am pretty sure that these do get sent. What does not get sent is an initial cork command at startup. I can add that easily, but, pavucontrol seems to keep an audio stream open all the time. libpipewire-module-qubes.so is just relaying the events PipeWire provides it, so I think this part is a bug in either PipeWire or pavucontrol.

when pipewire is started first, then some client (trying to) play something and only then pacat-simple-vchan, then log has endless stream of Overrun: asled tp wrote 2048 bytes, but can only write 0

I have not managed to reproduce this yet.

when recording is started in VM first, but recording is allowed later, there is a stream of Underrun: asked to read 2048 bytes, but only xx available (where xx is mostly non-zero, but sometimes 0). At ~~one case~~ two cases, when I stopped recording in the VM (closed the client), then pipewire crashed with SIGABRT and corrupted size vs prev_size message (I have a backtrace and coredump, but I'm not sure how useful they will be in practice)

Please send both to me privately.

DemiMarie · 2022-08-01T09:43:28Z

the module seems to not send playback start/stop commands, instead it sends silence (zeroes) when nothing is playing -> this prevents the actual device to suspend when unused, and results in constant CPU usage on the pacat-simple-vchan side

Fixed by reporting the current stream state on all vchan connections.

when pipewire is started first, then some client (trying to) play something and only then pacat-simple-vchan, then log has endless stream of Overrun: asled tp wrote 2048 bytes, but can only write 0

Hopefully this is fixed as well.

DemiMarie · 2022-08-01T10:02:30Z

when recording is started in VM first, but recording is allowed later, there is a stream of Underrun: asked to read 2048 bytes, but only xx available (where xx is mostly non-zero, but sometimes 0). At ~~one case~~ two cases, when I stopped recording in the VM (closed the client), then pipewire crashed with SIGABRT and corrupted size vs prev_size message (I have a backtrace and coredump, but I'm not sure how useful they will be in practice)

I get occasional underruns, but they stop after a few seconds. No crashes so far.

marmarek · 2022-08-01T14:03:47Z

CI:

qubes-pw-module.c: In function 'playback_stream_process':
qubes-pw-module.c:643:22: error: unused variable 'control_vchan' [-Werror=unused-variable]
  643 |     struct libvchan *control_vchan = capture_stream->vchan;
      |                      ^~~~~~~~~~~~~

marmarek · 2022-08-01T14:57:11Z

Delay acknowledgement of incoming events

(I see you reverted already, but I'll comment anyway)
If you watch vchan with epoll(() or similar, you need to call libvchan_wait() when processing an event, before any other libvchan_* function. Otherwise you may miss some events (because libvchan_wait() might acknowledge some later state change, so it won't be reported by epoll()).

But changing the order on its own should not cause SEGV - if that happens, you either called libvchan_close() in between, or there is some other bug.

DemiMarie · 2022-08-01T21:50:23Z

PipelineRetryFailed

Several keys were missing.

Just use "qubes-audio".

pacat-simple-vchan needs to know the current state of the streams upon vchan connection. Otherwise, the playback stream will not be corked and pacat-simple-vchan will consume CPU for no good reason. Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Assertions are not an error handling mechanism. If creating a vchan fails, or if the vchan cannot be registered, do something other than asserting.

xruns are a warning, not an error.

This caused infrequent reconnection failures.

If a stream state change has been requested, immediately after vchan connection is a good time to honor that request.

An overrun could be due to a lost uncork command. Send a fresh uncork command in that case.

Not having a default is unnecessarily annoying, and setting the size in the configuration file makes overriding it more difficult than it would be otherwise. Suggested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

This causes a SIGSEGV; will probably revert.

This is necessary to support loading the module multiple times.

Even a dead stream needs to be shut down properly.

stream->impl can be NULL in this case.

It turns out to only need 320 bytes.

The only time this can fail is if SPA buffers are too small. If this happens, it should be caught during testing.

Suggested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Wim Taymans's line cannot be changed, but mine can.

INT_MAX/2 (0x3FFFFFFF) should be enough for everyone :). This avoids a Xen-specific limit.

struct impl is shared by multiple threads simultaneously, and updates to a bitfield cannot be made atomic. Better to use ordinary boolean values in case something needs to be made atomic in the future.

Otherwise we will block the realtime thread.

Otherwise the two could conflict.

This allows it to be managed by Qubes.

This ensures that 'make install' works if run twice.

It makes no sense to _not_ have the PipeWire agent depend on PipeWire, as without PipeWire it is useless.

The PipeWire agent is actually slightly more code than the PulseAudio agent, but the benefits more than outweigh this.

DemiMarie force-pushed the pipewire branch from e1313d4 to 2b5703c Compare June 30, 2022 21:07

DemiMarie marked this pull request as ready for review July 1, 2022 06:09

marmarek reviewed Jul 2, 2022

View reviewed changes

pipewire/GNUmakefile Outdated Show resolved Hide resolved

DemiMarie mentioned this pull request Jul 4, 2022

Allow PipeWire to run QubesOS/qubes-core-agent-linux#388

Closed

DemiMarie force-pushed the pipewire branch 2 times, most recently from 8017ce2 to e3ef956 Compare July 5, 2022 05:22

DemiMarie force-pushed the pipewire branch from 089805a to 6347c60 Compare July 5, 2022 21:53

DemiMarie requested a review from marmarek July 5, 2022 22:23

DemiMarie force-pushed the pipewire branch from 6347c60 to 98f9750 Compare July 12, 2022 10:06

DemiMarie force-pushed the pipewire branch 2 times, most recently from 79840e9 to f6e0a75 Compare July 22, 2022 08:28

marmarek requested changes Jul 28, 2022

View reviewed changes

DemiMarie commented Jul 28, 2022

View reviewed changes

marmarek requested changes Jul 28, 2022

View reviewed changes

debian/control Outdated Show resolved Hide resolved

rpm_spec/gui-agent.spec.in Outdated Show resolved Hide resolved

rpm_spec/gui-agent.spec.in Outdated Show resolved Hide resolved

debian/qubes-vm-pipewire.postinst Outdated Show resolved Hide resolved

DemiMarie requested a review from marmarek July 28, 2022 20:50

marmarek reviewed Jul 30, 2022

View reviewed changes

pipewire/qubes-pw-module.c Outdated Show resolved Hide resolved

DemiMarie force-pushed the pipewire branch from 5232802 to 9ea9537 Compare August 1, 2022 14:43

DemiMarie added 25 commits April 14, 2023 13:04

Update usage information

53252f6

Several keys were missing.

Shorten the name of the module

eeaec6c

Just use "qubes-audio".

Do not rely on asserts for error handling

36b24d4

Assertions are not an error handling mechanism. If creating a vchan fails, or if the vchan cannot be registered, do something other than asserting.

Only use pw_log_error() for true errors

f8b8bdc

xruns are a warning, not an error.

Do not disable streams if control vchan is closed

28eb0d6

This caused infrequent reconnection failures.

Sync stream states after vchan open

ffbb586

If a stream state change has been requested, immediately after vchan connection is a good time to honor that request.

Process control commands if an overrun occurs

0223d63

An overrun could be due to a lost uncork command. Send a fresh uncork command in that case.

Provide default buffer sizes

e7e8659

Not having a default is unnecessarily annoying, and setting the size in the configuration file makes overriding it more difficult than it would be otherwise. Suggested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Use common code for stream shutdown

90c2d0d

This causes a SIGSEGV; will probably revert.

Remove shutting_down global variable

53f21df

This is necessary to support loading the module multiple times.

Shut down dead streams

7e1c6ec

Even a dead stream needs to be shut down properly.

Fix NULL pointer dereference if vchan connection fails

aece0a6

stream->impl can be NULL in this case.

Use a smaller SPA builder buffer

62096af

It turns out to only need 320 bytes.

Assert that SPA builder succeeded

75cda13

The only time this can fail is if SPA buffers are too small. If this happens, it should be caught during testing.

Debian packaging fixups

88c0bb9

Suggested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Allow domain IDs up to INT_MAX/2

5998085

INT_MAX/2 (0x3FFFFFFF) should be enough for everyone :). This avoids a Xen-specific limit.

Avoid using bitfields

cfe9236

struct impl is shared by multiple threads simultaneously, and updates to a bitfield cannot be made atomic. Better to use ordinary boolean values in case something needs to be made atomic in the future.

Check that atomics are lock-free

147038f

Otherwise we will block the realtime thread.

Turn off PulseAudio if PipeWire is enabled

1a02811

Otherwise the two could conflict.

Advertise the PipeWire service to dom0

ffed70d

This allows it to be managed by Qubes.

Fix 'make install'

9a5d494

This ensures that 'make install' works if run twice.

PipeWire agent: Add dependency on PipeWire

5dceb10

It makes no sense to _not_ have the PipeWire agent depend on PipeWire, as without PipeWire it is useless.

PipeWire agent is not less code

ab5cd1f

The PipeWire agent is actually slightly more code than the PulseAudio agent, but the benefits more than outweigh this.

DemiMarie force-pushed the pipewire branch from 666875a to ab5cd1f Compare April 14, 2023 17:05

marmarek added the openqa-pending label Apr 15, 2023

marmarek approved these changes Apr 26, 2023

View reviewed changes

marmarek merged commit 089e62d into QubesOS:master Apr 26, 2023

DemiMarie deleted the pipewire branch April 26, 2023 15:02

Uh oh!

Pipewire agent #157

Pipewire agent #157

Uh oh!

Conversation

DemiMarie commented Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marmarek commented May 4, 2022

Uh oh!

DemiMarie commented Jun 27, 2022

Uh oh!

DemiMarie commented Jul 1, 2022

Uh oh!

DemiMarie commented Jul 2, 2022

Uh oh!

Uh oh!

DemiMarie commented Jul 5, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marmarek Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

DemiMarie Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DemiMarie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DemiMarie Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marmarek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marmarek commented Jul 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marmarek commented Jul 31, 2022

Uh oh!

DemiMarie commented Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DemiMarie commented Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DemiMarie commented Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marmarek commented Aug 1, 2022

Uh oh!

marmarek commented Aug 1, 2022

Uh oh!

DemiMarie commented Aug 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

DemiMarie commented Apr 29, 2022 •

edited

Loading

marmarek commented Jul 31, 2022 •

edited

Loading

DemiMarie commented Aug 1, 2022 •

edited

Loading

DemiMarie commented Aug 1, 2022 •

edited

Loading

DemiMarie commented Aug 1, 2022 •

edited

Loading