Implement VirtIO sound device capture#115
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
54a45b5 to
05b22f9
Compare
2db638a to
781ef0a
Compare
| static void __virtio_snd_rx_frame_dequeue(void *out, | ||
| uint32_t n, | ||
| uint32_t stream_id) |
There was a problem hiding this comment.
__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);
This comment was marked as resolved.
This comment was marked as resolved.
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
30d1ccd to
41fe790
Compare
There was a problem hiding this comment.
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.
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' |
3117075 to
71e1db1
Compare
jserv
left a comment
There was a problem hiding this comment.
VSND_GEN_RX_QUEUE_HANDLER is 116 lines of macro that's 95% identical to VSND_GEN_TX_QUEUE_HANDLER. This is not "good taste."
|
commit message:
You're adding 400+ lines of code that "usually doesn't work"? This should be a draft/experimental branch, not a merge candidate. If it doesn't work, why are we adding CI tests that codify broken behavior? |
|
Missing Lock on TX Notify Path: case VSND_QUEUE_TX:
tx_ev_notify++; // NO LOCK!
pthread_cond_signal(&virtio_snd_tx_cond);
break;
// virtio-snd.c:1455 - RX path (new code)
case VSND_QUEUE_RX:
pthread_mutex_lock(&virtio_snd_rx_mutex); // HAS LOCK
rx_ev_notify++;
pthread_cond_signal(&virtio_snd_rx_cond);
pthread_mutex_unlock(&virtio_snd_rx_mutex);
break;Inconsistent synchronization patterns between TX and RX. TX is already racy - RX is correct but highlights the TX bug. |
There was a problem hiding this comment.
3 issues found across 1 file (changes from recent commits).
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="virtio-snd.c">
<violation number="1" location="virtio-snd.c:1132">
P1: Using `pthread_mutex_trylock` silently drops audio frames when the lock is unavailable. This will cause audio data loss and artifacts in the capture stream. Consider either using blocking `pthread_mutex_lock` with a timeout, or implementing a lock-free ring buffer for the real-time audio callback.</violation>
<violation number="2" location="virtio-snd.c:1133">
P2: Debug `fprintf` in the real-time audio callback path should be removed. Calling `fprintf` from a PortAudio callback can cause audio glitches as it may block.</violation>
<violation number="3" location="virtio-snd.c:1152">
P2: Debug `fprintf` in the real-time audio callback path should be removed.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| pthread_cond_signal(&props->lock.readable); | ||
|
|
||
| rx_frame_enque_finally: | ||
| fprintf(stderr, "enque end\n"); |
There was a problem hiding this comment.
P2: Debug fprintf in the real-time audio callback path should be removed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At virtio-snd.c, line 1152:
<comment>Debug `fprintf` in the real-time audio callback path should be removed.</comment>
<file context>
@@ -1119,29 +1126,32 @@ static void __virtio_snd_rx_frame_enqueue(const void *payload,
-rx_frame_enque_finally:
- pthread_mutex_unlock(&props->lock.lock);
+ rx_frame_enque_finally:
+ fprintf(stderr, "enque end\n");
+ pthread_mutex_unlock(&props->lock.lock);
+ }
</file context>
| while (props->lock.buf_ev_notity > 0) | ||
| pthread_cond_wait(&props->lock.writable, &props->lock.lock);*/ | ||
| if (pthread_mutex_trylock(&props->lock.lock) == 0) { | ||
| fprintf(stderr, "enque start \n"); |
There was a problem hiding this comment.
P2: Debug fprintf in the real-time audio callback path should be removed. Calling fprintf from a PortAudio callback can cause audio glitches as it may block.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At virtio-snd.c, line 1133:
<comment>Debug `fprintf` in the real-time audio callback path should be removed. Calling `fprintf` from a PortAudio callback can cause audio glitches as it may block.</comment>
<file context>
@@ -1119,29 +1126,32 @@ static void __virtio_snd_rx_frame_enqueue(const void *payload,
- list_push(&node->q, &props->buf_queue_head);
+ pthread_cond_wait(&props->lock.writable, &props->lock.lock);*/
+ if (pthread_mutex_trylock(&props->lock.lock) == 0) {
+ fprintf(stderr, "enque start \n");
+ /* Add a PCM frame to queue */
+ vsnd_buf_queue_node_t *node = malloc(sizeof(*node));
</file context>
| /*pthread_mutex_lock(&props->lock.lock); | ||
| while (props->lock.buf_ev_notity > 0) | ||
| pthread_cond_wait(&props->lock.writable, &props->lock.lock);*/ | ||
| if (pthread_mutex_trylock(&props->lock.lock) == 0) { |
There was a problem hiding this comment.
P1: Using pthread_mutex_trylock silently drops audio frames when the lock is unavailable. This will cause audio data loss and artifacts in the capture stream. Consider either using blocking pthread_mutex_lock with a timeout, or implementing a lock-free ring buffer for the real-time audio callback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At virtio-snd.c, line 1132:
<comment>Using `pthread_mutex_trylock` silently drops audio frames when the lock is unavailable. This will cause audio data loss and artifacts in the capture stream. Consider either using blocking `pthread_mutex_lock` with a timeout, or implementing a lock-free ring buffer for the real-time audio callback.</comment>
<file context>
@@ -1119,29 +1126,32 @@ static void __virtio_snd_rx_frame_enqueue(const void *payload,
- node->pos = 0;
- list_push(&node->q, &props->buf_queue_head);
+ pthread_cond_wait(&props->lock.writable, &props->lock.lock);*/
+ if (pthread_mutex_trylock(&props->lock.lock) == 0) {
+ fprintf(stderr, "enque start \n");
+ /* Add a PCM frame to queue */
</file context>
782e188 to
93ebde8
Compare
| #include "virtio.h" | ||
|
|
||
| #define VSND_DEV_CNT_MAX 1 | ||
| #define VSND_DEV_CNT_MAX 2 |
There was a problem hiding this comment.
Provide comments to explain.
There was a problem hiding this comment.
This PR is going to add capture device. As each VirtIO sound device can have only one direction when initialization (VIRTIO_SND_D_OUTPUT or VIRTIO_SND_D_INPUT), I set VSND_DEV_CNT_MAX to 2 (one for playback, and the other for capture).
| err = Pa_OpenStream(&props->pa_stream, ¶ms, NULL /* no output */, | ||
| rate, cnfa_period_frames, paClipOff, | ||
| virtio_snd_rx_stream_cb, &props->v); | ||
| if (err != paNoError) | ||
| goto pa_err; |
There was a problem hiding this comment.
Can you provide early diagnosis when PortAudio fails?
a40042a to
07c7618
Compare
055f60a to
429d9d2
Compare
429d9d2 to
a5b8265
Compare
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
Summary by cubic
Implements VirtIO sound capture (RX) with PortAudio input and full RX virtqueue support so mic audio reaches the guest. Adds wake-fd IRQ signaling and keeps TX/RX fully split with separate paths and threads.
Written for commit 352b5e2. Summary will update on new commits.