-
Notifications
You must be signed in to change notification settings - Fork 64
Implement VirtIO sound device capture #115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
jserv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebase to avoid unnecessary merge commits.
This comment was marked as resolved.
This comment was marked as resolved.
54a45b5 to
05b22f9
Compare
2db638a to
781ef0a
Compare
virtio-snd.c
Outdated
| static void __virtio_snd_rx_frame_enqueue(void *payload, | ||
| uint32_t n, | ||
| uint32_t stream_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__virtio_snd_rx_frame_enqueue() never increments buf_ev_notity or signals the condition variable, but __virtio_snd_rx_frame_dequeue() waits on it:
// In __virtio_snd_rx_frame_dequeue:
while (props->lock.buf_ev_notity < 1)
pthread_cond_wait(&props->lock.readable, ...); // BLOCKS FOREVERThe TX path correctly does:
props->lock.buf_ev_notity++;
pthread_cond_signal(&props->lock.readable);Fix: Add after list_push() in __virtio_snd_rx_frame_enqueue:
props->lock.buf_ev_notity++;
pthread_cond_signal(&props->lock.readable);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The buf_ev_notity will be increased and then signals the update of CV after consuming all the virtq elements when calling virtio_snd_rx_desc_normal_handler():
// In VSND_GEN_RX_QUEUE_HANDLER macro
virtio_snd_prop_t *props = &vsnd_props[stream_id];
props->lock.buf_ev_notity--;
pthread_cond_signal(&props->lock.writable);The virtio_snd_tx_desc_normal_handler() also has the same design.
The reason is that the driver should populate a buffer size of PCM frames (except the end of stream) as mentioned by VirtIO standard:
5.14.6.8.2.2 Driver Requirements: Input Stream
- The driver SHOULD populate the rx queue with period_bytes sized empty buffers before starting the stream.
- The driver MUST NOT place device-readable buffers into the rx queue.
| static void __virtio_snd_rx_frame_dequeue(void *out, | ||
| uint32_t n, | ||
| uint32_t stream_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__virtio_snd_rx_frame_dequeue() does list_del(&node->q) but never free(node):
if (node->pos >= node->len)
list_del(&node->q); // Missing: free(node);|
Debug Printf Pollution |
be341b2 to
1a14913
Compare
|
Can we perform loopback tests with virtio sound device? |
You mean |
The headless verification is crucial for CI/CD integration. |
fd4f538 to
a2348ec
Compare
Implement VirtIO sound device capture, with adding notice that the capture usually doesn't work for the reason of 'semu' emulation part.
30d1ccd to
41fe790
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 issues found across 4 files
Prompt for AI agents (all 3 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="virtio-snd.c">
<violation number="1" location="virtio-snd.c:373">
P2: The global `rx_ev_start` has a data race - it's read in the RX thread (in `__virtio_snd_rx_frame_dequeue`) while being written from the control path without synchronization. Consider using atomic operations or protecting access with `props->lock.lock` mutex.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:17">
P3: Grammar error: "until `semu` being rebooted" should be "until `semu` is rebooted".</violation>
<violation number="2" location="README.md:21">
P3: Grammar error: subject-verb agreement. "ALSA usually get stuck" should be "ALSA usually gets stuck".</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| static int rx_ev_notify; | ||
|
|
||
| // FIXME: set these variables into each capture stream;s structure | ||
| static int rx_ev_start; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: The global rx_ev_start has a data race - it's read in the RX thread (in __virtio_snd_rx_frame_dequeue) while being written from the control path without synchronization. Consider using atomic operations or protecting access with props->lock.lock mutex.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At virtio-snd.c, line 373:
<comment>The global `rx_ev_start` has a data race - it's read in the RX thread (in `__virtio_snd_rx_frame_dequeue`) while being written from the control path without synchronization. Consider using atomic operations or protecting access with `props->lock.lock` mutex.</comment>
<file context>
@@ -316,9 +362,15 @@ static virtio_snd_prop_t vsnd_props[VSND_DEV_CNT_MAX] = {
+static int rx_ev_notify;
+
+// FIXME: set these variables into each capture stream;s structure
+static int rx_ev_start;
/* vsnd virtq callback type */
</file context>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 issues found across 4 files
Prompt for AI agents (all 5 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="README.md">
<violation number="1" location="README.md:17">
P3: Grammar issue: 'stuck' should be 'to get stuck' and 'being rebooted' should be 'is rebooted'.</violation>
<violation number="2" location="README.md:21">
P3: Subject-verb agreement error: "ALSA usually get stuck" should be "ALSA usually gets stuck".</violation>
</file>
<file name=".github/workflows/main.yml">
<violation number="1" location=".github/workflows/main.yml:72">
P1: The sound test runs unconditionally for all matrix variants, including when `matrix.dependency == 'none'` where no sound library is installed. This will likely cause the test to fail. Consider adding a condition similar to the install step.</violation>
</file>
<file name="virtio-snd.c">
<violation number="1" location="virtio-snd.c:857">
P1: Potential double-free: `props->intermediate` is freed in error path but not set to NULL. If `virtio_snd_read_pcm_release` is called later (state is already PREPARE), it will free the same pointer again.</violation>
<violation number="2" location="virtio-snd.c:1225">
P1: Missing NULL checks after malloc. If memory allocation fails, dereferencing `node` or `node->addr` will cause a crash.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| run: sudo .ci/test-netdev.sh | ||
| shell: bash | ||
| timeout-minutes: 10 | ||
| - name: sound test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: The sound test runs unconditionally for all matrix variants, including when matrix.dependency == 'none' where no sound library is installed. This will likely cause the test to fail. Consider adding a condition similar to the install step.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/main.yml, line 72:
<comment>The sound test runs unconditionally for all matrix variants, including when `matrix.dependency == 'none'` where no sound library is installed. This will likely cause the test to fail. Consider adding a condition similar to the install step.</comment>
<file context>
@@ -69,6 +69,10 @@ jobs:
run: sudo .ci/test-netdev.sh
shell: bash
timeout-minutes: 10
+ - name: sound test
+ run: .ci/test-sound.sh
+ shell: bash
</file context>
| - name: sound test | |
| - name: sound test | |
| if: matrix.dependency != 'none' |
jserv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refine git commit messages with explicit limitations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 4 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name=".ci/test-sound.sh">
<violation number="1" location=".ci/test-sound.sh:47">
P1: Exit code is captured from `echo` instead of `test_sound`. With `set -e`, if `test_sound` fails the script exits immediately, making the error messages dead code. The exit status should be captured right after `test_sound`.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:16">
P3: Grammatical issue: The sentence structure is broken. "As a confirmed issue that... causing... will get stuck" doesn't form a proper sentence. Consider rephrasing to: "Due to a confirmed issue where `semu` cannot send/receive PCM frames in time, the ALSA stack may get stuck in XRUN state until you reboot `semu`."</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| test_sound | ||
| echo "✓ sound test passed" | ||
|
|
||
| ret="$?" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Exit code is captured from echo instead of test_sound. With set -e, if test_sound fails the script exits immediately, making the error messages dead code. The exit status should be captured right after test_sound.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .ci/test-sound.sh, line 47:
<comment>Exit code is captured from `echo` instead of `test_sound`. With `set -e`, if `test_sound` fails the script exits immediately, making the error messages dead code. The exit status should be captured right after `test_sound`.</comment>
<file context>
@@ -0,0 +1,62 @@
+test_sound
+echo "✓ sound test passed"
+
+ret="$?"
+
+MESSAGES=("OK!" \
</file context>
| - As a confirmed issue that `semu` cannot send/receive PCM frames in time, causing | ||
| the ALSA stack will get stuck in XRUN state until you reboot `semu`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3: Grammatical issue: The sentence structure is broken. "As a confirmed issue that... causing... will get stuck" doesn't form a proper sentence. Consider rephrasing to: "Due to a confirmed issue where semu cannot send/receive PCM frames in time, the ALSA stack may get stuck in XRUN state until you reboot semu."
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 16:
<comment>Grammatical issue: The sentence structure is broken. "As a confirmed issue that... causing... will get stuck" doesn't form a proper sentence. Consider rephrasing to: "Due to a confirmed issue where `semu` cannot send/receive PCM frames in time, the ALSA stack may get stuck in XRUN state until you reboot `semu`."</comment>
<file context>
@@ -12,12 +12,13 @@ A minimalist RISC-V system emulator capable of running Linux the kernel and corr
- to adjust the buffer size to more than four times of period size, or
- the program cannot write the PCM frames into guest OS ALSA stack.
+ - virtio-snd uses [PortAudio](https://github.com/PortAudio/portaudio) for sound playback and capture on the host with one limitation:
+ - As a confirmed issue that `semu` cannot send/receive PCM frames in time, causing
+ the ALSA stack will get stuck in XRUN state until you reboot `semu`.
+ - For playback, you can try to adjust the buffer size to more than four times of period size.
</file context>
| - As a confirmed issue that `semu` cannot send/receive PCM frames in time, causing | |
| the ALSA stack will get stuck in XRUN state until you reboot `semu`. | |
| - Due to a confirmed issue where `semu` cannot send/receive PCM frames in time, | |
| the ALSA stack may get stuck in XRUN state until you reboot `semu`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 4 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="README.md">
<violation number="1" location="README.md:16">
P3: This sentence has grammatical issues that make it hard to parse. Consider restructuring to: "Due to a confirmed issue, `semu` cannot send/receive PCM frames in time, causing the ALSA stack to get stuck in XRUN state until you reboot `semu`."</violation>
</file>
<file name="virtio-snd.c">
<violation number="1" location="virtio-snd.c:1230">
P1: PortAudio callbacks must be non-blocking. `__virtio_snd_rx_frame_enqueue` uses `pthread_cond_wait` which can block indefinitely when called from the real-time audio callback `virtio_snd_rx_stream_cb`. This can cause audio glitches, buffer underruns, or deadlocks. Consider using a lock-free ring buffer or non-blocking queue for the audio callback path.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| to adjust the buffer size to more than four times of period size, or | ||
| the program cannot write the PCM frames into guest OS ALSA stack. | ||
| - virtio-snd uses [PortAudio](https://github.com/PortAudio/portaudio) for sound playback and capture on the host with one limitation: | ||
| - As a confirmed issue that `semu` cannot send/receive PCM frames in time, causing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3: This sentence has grammatical issues that make it hard to parse. Consider restructuring to: "Due to a confirmed issue, semu cannot send/receive PCM frames in time, causing the ALSA stack to get stuck in XRUN state until you reboot semu."
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 16:
<comment>This sentence has grammatical issues that make it hard to parse. Consider restructuring to: "Due to a confirmed issue, `semu` cannot send/receive PCM frames in time, causing the ALSA stack to get stuck in XRUN state until you reboot `semu`."</comment>
<file context>
@@ -12,12 +12,13 @@ A minimalist RISC-V system emulator capable of running Linux the kernel and corr
- to adjust the buffer size to more than four times of period size, or
- the program cannot write the PCM frames into guest OS ALSA stack.
+ - virtio-snd uses [PortAudio](https://github.com/PortAudio/portaudio) for sound playback and capture on the host with one limitation:
+ - As a confirmed issue that `semu` cannot send/receive PCM frames in time, causing
+ the ALSA stack will get stuck in XRUN state until you reboot `semu`.
+ - For playback, you can try to adjust the buffer size to more than four times of period size.
</file context>
| - As a confirmed issue that `semu` cannot send/receive PCM frames in time, causing | |
| - Due to a confirmed issue, `semu` cannot send/receive PCM frames in time, causing |
| virtio_snd_prop_t *props = &vsnd_props[stream_id]; | ||
|
|
||
| pthread_mutex_lock(&props->lock.lock); | ||
| while (props->lock.buf_ev_notity > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: PortAudio callbacks must be non-blocking. __virtio_snd_rx_frame_enqueue uses pthread_cond_wait which can block indefinitely when called from the real-time audio callback virtio_snd_rx_stream_cb. This can cause audio glitches, buffer underruns, or deadlocks. Consider using a lock-free ring buffer or non-blocking queue for the audio callback path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At virtio-snd.c, line 1230:
<comment>PortAudio callbacks must be non-blocking. `__virtio_snd_rx_frame_enqueue` uses `pthread_cond_wait` which can block indefinitely when called from the real-time audio callback `virtio_snd_rx_stream_cb`. This can cause audio glitches, buffer underruns, or deadlocks. Consider using a lock-free ring buffer or non-blocking queue for the audio callback path.</comment>
<file context>
@@ -952,6 +1220,37 @@ static void __virtio_snd_frame_enqueue(void *payload,
+ virtio_snd_prop_t *props = &vsnd_props[stream_id];
+
+ pthread_mutex_lock(&props->lock.lock);
+ while (props->lock.buf_ev_notity > 0)
+ pthread_cond_wait(&props->lock.writable, &props->lock.lock);
+
</file context>
Summary by cubic
Implements VirtIO sound capture (RX) with PortAudio input, RX virtqueue handling, and circular buffering so input audio can be delivered to the guest. Also exposes separate input/output streams with accurate device info.
Written for commit a5cbdc1. Summary will update automatically on new commits.