Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a virtual tuner daemon and server to Cuttlefish, including an audio generator, a PCM stream server, and a gRPC control service, along with their integration into the run_cvd flow and associated unit tests. The review feedback highlights several critical issues, including potential null pointer dereferences when checking if potentially empty SharedFDs are open, a dangerous file deletion risk in test teardown if the temporary directory is empty, and potential uninitialized memory leaks if the audio buffer size is not a multiple of the channel count.
| return WriteAll(fd, buf.data(), buf.size()); | ||
| } | ||
|
|
||
| bool SendAll(SharedFD sock, std::string_view msg) { |
There was a problem hiding this comment.
Consider absl::Span instead, perhaps.
There was a problem hiding this comment.
A Span<const char> would still need the cast and byte count at the call site, so I kept pointer+size to match Fd::Send. Happy to add a typed Span<const T> overload if you prefer.
| namespace cuttlefish { | ||
| namespace { | ||
|
|
||
| constexpr unsigned int kVirtualTunerVsockPort = 7010; |
There was a problem hiding this comment.
Using a fixed port like this breaks multi-device functionality. Each device needs to select its port from a range by this formula: first_port + instance_num - 1. Look at how the adb host port is chosen in host/commands/assemble_cvd/flags.cc for an example.
These ranges usually cover 100 ports, [7000..7099] and [7100..7199] are available in the vsock address space.
There was a problem hiding this comment.
In case of the vhost-user-vsock, it turns a guest connection to (CID 2, port P) into a Unix socket connection to a file in that instance's directory: /tmp/vsock-CID-UID/vm.vsock-PORT. Therefore, it worked before.
But I agree that it's better to select the port automatically and then pass it to the guest through bootconfig. Done in the next patch.
008bcdb to
666251d
Compare
Introduce the audio emulation pipeline for the Cuttlefish Virtual Tuner: - AudioGenerator: Generates 48 kHz, 16-bit stereo PCM audio frames (white noise while tuned/playing, silence while stopped/untuned). - PcmStreamServer: Multi-client PCM stream server delivering 48 kHz 16-bit stereo audio frames over a UNIX domain stream socket to Cuttlefish's virtio-snd virtual sound card backend (CrosVM). - Unit and integration tests covering audio frame synthesis and socket streaming. Test: bazel test //cuttlefish/host/commands/virtual_tuner_daemon:... Bug: 521329213 TAG=agy CONV=809829e8-ac68-4c32-b9b7-632f61743e47
Introduce the VirtualTunerServiceImpl gRPC control service and the virtual_tuner_daemon binary entry point: - VirtualTunerServiceImpl: Implements Tune and Stop RPCs updating TunerState. - virtual_tuner_daemon: Runs PcmStreamServer alongside the gRPC control server. It takes --server_address, --pcm_server_fd and --pcm_channels. run_cvd creates the PCM listening socket before starting WebRTC, because WebRTC connects to it only once. --pcm_channels is the channel count of the guest's virtual_tuner audio stream; anything other than 2 is rejected because the generator produces stereo. The launcher is expected to validate the guest config first, so this check is a backstop. - Unit tests verifying Tune and Stop RPCs over an in-process gRPC channel. Test: bazel test //cuttlefish/host/commands/virtual_tuner_daemon/... Bug: b/521329213 TAG=agy CONV=809829e8-ac68-4c32-b9b7-632f61743e47
Moves AudioChannelsLayout and GetChannelsCount out of webRTC's audio_settings.h, and ConvertChannelLayout out of webRTC's main.cpp, into a new library, //cuttlefish/host/libs/audio:channel_layout. That lets run_cvd turn a guest audio config layout into a channel count without copying webRTC's mapping. audio_settings.h re-exports the new header, so the rest of webRTC is unchanged. No behavior change. Test: bazel test //cuttlefish/host/libs/audio:all //cuttlefish/host/frontend/webrtc:all Bug: b/521329213 TAG=agy CONV=e89104c1-07ca-48c1-bafc-c87fbf95b6df
When the guest audio config declares a virtual_tuner stream on any PCM device, run_cvd now starts virtual_tuner_daemon and passes webRTC the path of the tuner PCM socket. Guests without a virtual_tuner stream don't start the daemon. run_cvd creates the PCM listening socket and hands it to the daemon with --pcm_server_fd, so the socket exists before webRTC starts: webRTC's TunerAudioSource connects once, at construction. The gRPC control server listens on a per-instance vsock port (7000 for instance 1, 7001 for instance 2, and so on). It goes through vhost-user vsock when that is enabled and through host AF_VSOCK otherwise. assemble_cvd assigns the port to every instance, as it does for the VHAL proxy server, stores it in the instance config and passes it to the guest as the androidboot.vsock_virtual_tuner_port bootconfig property. run_cvd derives --pcm_channels from the stream's channel layout with the helpers in host/libs/audio, and the daemon exits at startup if that isn't stereo. The daemon is not a critical process: if it exits, the device keeps running without tuner audio. Test: bazel test //cuttlefish/host/commands/run_cvd/launch:all //cuttlefish/host/commands/assemble_cvd:all //cuttlefish/host/libs/config:all //cuttlefish/package:cvd; cvd load on cf_x86_64_auto with a virtual_tuner stream, with vhost-user vsock and with host AF_VSOCK Bug: b/521329213 TAG=agy CONV=e89104c1-07ca-48c1-bafc-c87fbf95b6df
666251d to
5c396d8
Compare
This branch has not been deployed
3 participants
[continuation of #3175 and #3231]
When the guest audio config declares a virtual_tuner stream, run_cvd
starts virtual_tuner_daemon and passes webRTC --tuner_pcm_socket_path.
Guests without that stream get no socket, no daemon and no new flag.
run_cvd creates the PCM listening socket and hands it to the daemon
with --pcm_server_fd, because webRTC's TunerAudioSource connects once
at startup.
The gRPC control server listens on vsock port 7010 through vhost-user
vsock, so each instance gets its own socket. With kernel vsock every
instance would share host port 7010, so the launch fails when
vhost-user vsock is off.
--pcm_channels comes from the stream's channel layout. The daemon
exits at startup unless it's stereo.
The daemon isn't critical: if it exits, the device keeps running
without tuner audio.
Known limitations:
After a snapshot restore the daemon starts untuned, so the tuner is
silent until the guest tunes again.
If the daemon exits, tuner audio stays silent until the device
restarts, because webRTC doesn't reconnect.