Add support for QEMU's audio extension - #2138
Conversation
QEMU can send the guest's audio over the same vendor message type it already uses for extended key events, if the client asks for it by advertising a pseudo encoding. The server acknowledges by sending a rectangle with that encoding, and only then may the client choose a sample format and start the stream. Asking any earlier is an error that QEMU disconnects over, as the server has no audio device to record from unless it was started with one. Only the protocol is handled here. CConnection turns the incoming messages into calls a subclass can implement, and asks the subclass what sample format it wants, but does nothing with the samples itself. It also leaves the encoding unadvertised unless a subclass sets supportsAudio, since a server that offers audio will encode and send it purely because we asked, and there is no point paying for that when nothing can play it. The wire format, the constants, and the choice of -259 for the pseudo encoding are all from Mikhail Kupchik's implementation in pull request TigerVNC#1478, which has been waiting for review since 2022. The division of labour is different: that one gives CMsgHandler five audio specific methods, whereas this decodes the submessage in CConnection, so the reader and the handler only need to know that QEMU multiplexes several unrelated things on message type 255.
Adds a playback device abstraction and one implementation of it, using the classic waveOut API. That is enough for the formats this extension can carry and is available on every version of Windows we support. Other platforms compile but report that they have no way to play audio, and so never ask the server for any. Everything runs on the main loop, so none of this may block. Samples are copied into a circular buffer and handed to the mixer asynchronously. The completion callback runs on a thread belonging to the system, and does nothing but push the finished buffer onto a list for the main thread to unprepare and reuse later, as no waveOut call may be made from inside it. The playback follows Mikhail Kupchik's Win32AudioOutput from pull request TigerVNC#1478 closely, including playing a short period of silence ahead of a new stream, and lengthening that silence whenever the device is seen to run dry. It does not repeat that version's search over 16 combinations of sample width, channel count and frequency. WAVE_MAPPER converts formats for us, so a single one is asked for, and audio is reported as unavailable if even that is refused.
This restores what TigerVNC#1478 arrived at. Its final commit switched the output rate to 48 kHz "to avoid downsampling in QEMU for modern Windows guests", and narrowing the format search to a single request had quietly undone it. The comment justifying 44.1 kHz was wrong on the mechanism as well: QEMU resamples from whatever the guest produces, so there is no server-side default for the client to match. Asking for the rate modern guests already output is what avoids a conversion.
The waveOut backend made audio a Windows-only feature: AudioOutput::create() returned nullptr everywhere else, so a viewer on any other platform never advertised the pseudo-encoding and the server never sent anything. This adds a second backend on libpulse, which covers both PulseAudio and PipeWire systems since PipeWire's pulse server is what answers there. It is the asynchronous API driven by a pa_threaded_mainloop rather than pa_simple, because AudioOutput.h's own contract is that no method may block and pa_simple_write() blocks until the server has taken the data. The buffering is deliberately the same as the waveOut backend's, down to the constants: the same power-of-two circular buffer, the same drop when it fills, the same silence played ahead of a stream, and the same widening of that silence by however long the device was found to have run dry. Only the handover differs, since pa_stream_write() copies the samples out and PulseAudio reports starvation directly rather than by inference. Availability is decided at construction, as create() requires, but with a bounded wait so that a sound server which never answers cannot hold up the viewer's startup. Opening the stream is deferred to the first start() and does not wait at all. libpulse is a new dependency, and an optional one: ENABLE_AUDIO follows the pattern ENABLE_H264 already uses, so a build without libpulse compiles exactly as before and create() keeps returning nullptr. On Windows it is set unconditionally, waveOut being part of the OS.
|
Thanks for having another look at this. I don't normally use QEMU. Do you have a quick guide to point to to quickly get a test machine running? |
|
Of course — and thanks for taking a look. The one thing worth saying up front, because it looks exactly like the feature being broken: the audio comes from QEMU's own A minimal guest that is enough to test with — no OS install needed, any bootable ISO with a startup sound, or just run The load-bearing parts are Two things that cost me a few minutes, in case they save you the same: Then If you'd rather not run QEMU at all, I've put a small RFB server on a branch off this one that does the same job deterministically: https://github.com/jose-pr/tigervnc/tree/qemu-audio-bench/tests/audio It speaks 3.8/None, answers one FramebufferUpdateRequest with a rectangle carrying pseudo-encoding -259, reads The second line is the control arm: the identical session with the pseudo-encoding withheld, where the viewer must play nothing at all. Worth running, because without it a bench can't distinguish "audio works" from "the capture rig reports sound whatever happens". It also exits non-zero if audio was offered and never enabled, so a run that streamed nothing can't be mistaken for a green one. Standard library only, no dependencies, and written from the wire format — it shares no code with TigerVNC or any other VNC implementation, which is what makes agreement between it and the viewer worth something. Happy to fold it into this PR, send it as its own, or leave it on the branch as a testing aid — whichever you prefer. |
|
A quick test here just gives me a crash with Have you tried this with a debug build? The above guard rail isn't present on release builds. |
getptr() consumes the assured-data budget that the preceding hasData() established, so the following skip() had nothing left to spend and tripped the RFB_INSTREAM_CHECK assertion. Re-assert the length between the two, as JPEGDecoder.cxx already does for the same reason. The bytes were genuinely assured, so this was a spurious assertion rather than a real overrun, and it only fired on debug builds where RFB_INSTREAM_CHECK is defined.
|
Thanks — that's a real bug, and a good catch. You're right that I'd been testing a release build, which is exactly why I didn't see it: The cause is in if (!is->hasDataOrRestore(length)) // checkedBytes = length
return false;
is->clearRestorePoint();
handler->handleQEMUServerMessage(submessage, operation,
is->getptr(length), length); // check(length) -> checkedBytes = 0
is->skip(length); // check(length) -> throws
handler->handleQEMUServerMessage(submessage, operation,
is->getptr(length), length);
// getptr() resets the amount of assured data
is->hasData(length);
is->skip(length);For what it's worth on severity: the bytes really were assured by the preceding I've verified it against a debug build this time, driving the reader with synthesised AudioData messages: 8 of 9 cases fail before the change and all 9 pass after. That includes payloads dribbled in 1/3/7/64/1000-byte chunks, to force the re-entrant path where a message spans several reads — which I'd previously not been exercising at all, and which is the part I'd consider most likely to still hold a genuine bug. If it would help I can fold those cases in as a proper test rather than a throwaway harness. Pushed as 3fc7648. |
2 participants
QEMU can send a guest's audio to a VNC client over a vendor extension of its
own. This teaches the viewer to ask for it and to play it, so that connecting
to a QEMU guest gives you sound as well as a picture.
This is @mkupchik's work. He opened #1478 in June 2022 with a complete
implementation of this feature, got a review saying "This is definitely
something we are interested in" and a list of changes, and then it stalled.
It is still open and now conflicts with master. What is here is his design,
rebased onto current master, with the outstanding review comments addressed.
The playback engine in particular is his — the circular buffer, the silence
played ahead of a new stream, the lock-free handoff out of the device
callback — and his copyright is on the files that carry it.
I have opened this separately only because #1478 has had no commits from its
author since July 2022. If he is still around, or if you would rather this
went in as his, I would much prefer to send these as commits on top of #1478
and close this.
The review comments on #1478
Five things were asked for. Two were done, one partly, and two were not. The
two that were not are done here.
"please rename this to something like
supportsAudio" — partly donethere. Finished here as far as I think it should go: everything a subclass
sees is generic (
CConnection::supportsAudio,getAudioFormat(),handleAudioBegin(),handleAudioEnd(),handleAudioData()). I didnot rename
supportsQEMUAudio()onCMsgHandler, because it reportsone specific QEMU pseudo encoding arriving and sits directly beside the
existing
supportsQEMUKeyEvent(). Say the word if you would rather it weregeneric there too.
"Please try to define constants for things such as this" — done, in
qemuTypes.h."Can this be pushed to CConnection instead? Starting the audio on first
detection of QEMU audio support should be generic" — done;
CConnection::framebufferUpdateEnd()starts the handshake."This complexity is something that doesn't really fit well in CMsgReader
... Can't we let the input stream handle the buffering? ... We don't need
to support the theoretical extremes" — done.
readQEMUServerMessage()is 41 lines and holds no state of its own: read the submessage and the
operation, return early for anything that is not audio data, read the
length, refuse anything over 1 MiB with a
protocol_error,hasDataOrRestore(), handgetptr()straight to the handler,skip().MSGSTATE_AUDIO_DATA,nAudioBytesLeftandreadAudioData()are gone."Doesn't Windows resample for us? Do we really need to try all of these?
... Have you actually seen it failing to give you the requested format?" —
done. The search over 16 combinations of sample width, channel count
and frequency is gone. One format is asked for, and if the device refuses
it the viewer reports that it has no audio and never advertises the
encoding.
That one format is 48 kHz stereo S16, which is what QEMU Audio support in VNC Viewer (currently Windows only) #1478 itself
arrived at — its last commit was "Switched to 48 kHz output sample rate
... to avoid downsampling in QEMU for modern Windows guests". Narrowing the
search had quietly undone that; it is restored.
Shape
CMsgHandlergains two methods rather than five: one saying the server hasadvertised audio, and one passing a QEMU submessage on undecoded. Working out
that submessage 1, operation 2 means audio data happens in
CConnection,which is the layer that already knows what the extension is. The do-nothing
defaults live on
CConnectionbeside the clipboard ones, sinceCMsgHandleris pure virtual throughout.
Playback sits behind an
AudioOutputinterface shaped likeKeyboard.There are two backends:
waveOut— @mkupchik's, essentially unchanged.PipeWire systems since PipeWire's pulse server is what answers there.
The Pulse backend is the asynchronous API driven by a
pa_threaded_mainlooprather than
pa_simple, becauseAudioOutput.h's own contract is that nomethod may block and
pa_simple_write()blocks until the server has takenthe data. Its buffering is deliberately the same as the waveOut backend's,
constant for constant — the same circular buffer, the same drop when it
fills, the same silence ahead of a stream. Only the handover differs, since
pa_stream_write()copies the samples out and PulseAudio reports starvationdirectly rather than by inference.
libpulse is optional, via
trioption(ENABLE_AUDIO)in the shapeENABLE_H264already uses: a build without it compiles exactly as before andcreate()keeps returning nullptr. On Windows it is unconditional, waveOutbeing part of the OS.
There is an
Audioparameter, on by default, with a checkbox and a man pageentry. #1478 enabled audio unconditionally, with no way to turn it off.
What this does not do
AudioOutput::create()returns nullptr there and theviewer never advertises the encoding, exactly as on a Linux box built
without libpulse.
Reproducing it
Start QEMU with an audio device and tell the VNC server about it:
-audiodev noneis the point: the host plays nothing, it only captures forVNC. Then connect and make the guest play something.
Worth knowing if you go looking: QEMU only offers the extension when it was
started with an audio device, and it disconnects a client that sends an audio
message before the server has acknowledged the pseudo encoding. So the
acknowledgement rectangle has to gate the handshake.
Testing
The handshake, on both platforms:
Windows — heard. A 1 kHz sine tone, the ALSA spoken-channel samples, and
thirty seconds of continuous synthesised stereo music were played in a Linux
guest under QEMU and heard from the client's speakers. The music was panned
slowly between channels, so it also confirms the two channels carry different
content rather than one duplicated. No dropouts, and the viewer was still
connected at the end.
Linux — measured, not heard. The viewer ran on Rocky Linux 9 against the
same QEMU audio display, playing into a synthetic sink whose PCM was captured
and analysed. Against a server sending a sine it generated itself, the peak
was 0.610352 — 20000/32768 to the last digit. Halving the source
amplitude halved it to 0.305176; moving the tone moved the dominant frequency
bin. Against the real QEMU display the captured tone read 439.96–439.98 Hz at
0.610260 and 0.305084 for source amplitudes of 20000 and 10000, while the
kernel's own socket byte counter read 194,708 B/s against 192,000 B/s of
audio. No dropouts in any run.
The control matters as much as the signal: with
-Audio=0the capture isexact zeros for its whole length, the
-259pseudo encoding is absent fromSetEncodings, and the socket counter reads 0 B/s.Nobody listened on Linux, and I am not claiming otherwise — that run
supports "the backend handed the sound server exactly the samples it was
given, at the right rate, with no gaps", which is a narrower claim than the
Windows one.
Two paths in the Pulse backend are written and reviewed but never fired,
because the local network never starved the stream: the adaptive widening of
the pre-roll silence after an underflow, and the drop when the buffer fills.
The bounded connect timeout was likewise never hit, since the sound server
always answered immediately.
One caveat on the Windows side of this branch: it is based on upstream, which
declines to configure under MSVC, so the Windows build here was compiled
through a local opt-in that is not part of this pull request, and I have not
exercised the MinGW path your CI uses. Nothing in the audio code is compiler
specific —
<mmsystem.h>andwaveOut*, which mingw-w64 has — but I havenot proven that.