Skip to content

Commit d5c25b9

Browse files
pennycodersclaude
andcommitted
Fix HIGH priority issues in audio system
Addressed 5 HIGH priority issues identified in code review: HIGH jetkvm#12: safe_alsa_open validation (audio.c:314-327) - Added validation for snd_pcm_nonblock() return value - Properly close handle and return error if blocking mode fails - Prevents silent failures when switching to blocking mode HIGH jetkvm#13: WebRTC write failure handling (relay.go:74-147) - Track consecutive WebRTC write failures in OutputRelay - Reconnect source after 50 consecutive failures - Throttle warning logs (first failure + every 10th) - Prevents silent audio degradation from persistent write errors HIGH jetkvm#14: Opus packet size validation (audio.c:1024-1027) - Moved validation before mutex acquisition - Reduces unnecessary lock contention for invalid packets - Validates opus_buf, opus_size bounds before hot path HIGH jetkvm#15: FEC recovery validation (audio.c:1050-1071) - Added detailed logging for FEC usage - Log warnings when decode fails and FEC is attempted - Log info/error messages for FEC success/failure - Log warning when FEC returns 0 frames (silence) - Improves debuggability of packet loss scenarios Comment accuracy fixes (audio.c:63-67, 149-150, 164-165) - Clarified RFC 7587 comment: RTP clock rate vs codec sample rate - Explained why sr/fs parameters are ignored - Added context about SpeexDSP resampling Channel map validation (audio.c:572-579) - Added validation for unexpected channel counts - Check for SND_CHMAP_UNKNOWN positions - Prevents crashes from malformed channel map data 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 0f8b368 commit d5c25b9

File tree

2 files changed

+71
-15
lines changed

2 files changed

+71
-15
lines changed

internal/audio/c/audio.c

Lines changed: 48 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -60,9 +60,11 @@ static OpusEncoder *encoder = NULL;
6060
static OpusDecoder *decoder = NULL;
6161
static SpeexResamplerState *capture_resampler = NULL;
6262

63-
// Audio format - Opus always uses 48kHz for WebRTC (RFC 7587)
64-
static const uint32_t opus_sample_rate = 48000; // Opus RTP clock rate required to be 48kHz
65-
static uint32_t hardware_sample_rate = 48000; // Hardware-negotiated rate
63+
// Audio format - RFC 7587 requires Opus RTP clock rate (not sample rate) to be 48kHz
64+
// The Opus codec itself supports multiple sample rates (8/12/16/24/48 kHz), but the
65+
// RTP timestamp clock must always increment at 48kHz for WebRTC compatibility
66+
static const uint32_t opus_sample_rate = 48000; // RFC 7587: Opus RTP timestamp clock rate (not codec sample rate)
67+
static uint32_t hardware_sample_rate = 48000; // Hardware-negotiated rate (can be 44.1k, 48k, 96k, etc.)
6668
static uint8_t capture_channels = 2; // OUTPUT: Audio source (HDMI or USB) → client (stereo by default)
6769
static uint8_t playback_channels = 1; // INPUT: Client mono mic → device (always mono for USB audio gadget)
6870
static const uint16_t opus_frame_size = 960; // 20ms frames at 48kHz (fixed)
@@ -144,7 +146,8 @@ void update_audio_constants(uint32_t bitrate, uint8_t complexity,
144146
buffer_period_count = (buf_periods >= 2 && buf_periods <= 24) ? buf_periods : 12;
145147
opus_packet_loss_perc = (pkt_loss_perc <= 100) ? pkt_loss_perc : 20;
146148

147-
// Note: sr and fs parameters ignored - Opus always uses 48kHz with 960 samples
149+
// Note: sr and fs parameters ignored - RFC 7587 requires fixed 48kHz RTP clock rate
150+
// Hardware sample rate conversion is handled by SpeexDSP resampler
148151
}
149152

150153
void update_audio_decoder_constants(uint32_t sr, uint8_t ch, uint16_t fs, uint16_t max_pkt,
@@ -158,7 +161,8 @@ void update_audio_decoder_constants(uint32_t sr, uint8_t ch, uint16_t fs, uint16
158161
max_backoff_us_global = max_backoff > 0 ? max_backoff : 500000;
159162
buffer_period_count = (buf_periods >= 2 && buf_periods <= 24) ? buf_periods : 12;
160163

161-
// Note: sr and fs parameters ignored - always 48kHz with 960 samples
164+
// Note: sr and fs parameters ignored - decoder always operates at 48kHz (RFC 7587)
165+
// Playback device configured at 48kHz, no resampling needed for output
162166
}
163167

164168
/**
@@ -310,7 +314,16 @@ static int safe_alsa_open(snd_pcm_t **handle, const char *device, snd_pcm_stream
310314
while (attempt < max_attempts_global) {
311315
err = snd_pcm_open(handle, device, stream, SND_PCM_NONBLOCK);
312316
if (err >= 0) {
313-
snd_pcm_nonblock(*handle, 0);
317+
// Validate that we can switch to blocking mode
318+
err = snd_pcm_nonblock(*handle, 0);
319+
if (err < 0) {
320+
fprintf(stderr, "ERROR: Failed to set blocking mode on %s: %s\n",
321+
device, snd_strerror(err));
322+
fflush(stderr);
323+
snd_pcm_close(*handle);
324+
*handle = NULL;
325+
return err;
326+
}
314327
return 0;
315328
}
316329

@@ -556,7 +569,15 @@ static int configure_alsa_device(snd_pcm_t *handle, const char *device_name, uin
556569
if (num_channels == 2 && channels_swapped_out) {
557570
snd_pcm_chmap_t *chmap = snd_pcm_get_chmap(handle);
558571
if (chmap != NULL) {
559-
if (chmap->channels == 2) {
572+
if (chmap->channels != 2) {
573+
fprintf(stderr, "WARN: %s: Expected 2 channels but channel map has %u\n",
574+
device_name, chmap->channels);
575+
fflush(stderr);
576+
} else if (chmap->pos[0] == SND_CHMAP_UNKNOWN || chmap->pos[1] == SND_CHMAP_UNKNOWN) {
577+
fprintf(stderr, "WARN: %s: Channel map positions are unknown, cannot detect swap\n",
578+
device_name);
579+
fflush(stderr);
580+
} else {
560581
bool is_swapped = (chmap->pos[0] == SND_CHMAP_FR && chmap->pos[1] == SND_CHMAP_FL);
561582
if (is_swapped) {
562583
fprintf(stdout, "INFO: %s: Hardware reports swapped channel map (R,L instead of L,R)\n",
@@ -1000,6 +1021,11 @@ __attribute__((hot)) int jetkvm_audio_decode_write(void * __restrict__ opus_buf,
10001021
uint8_t recovery_attempts = 0;
10011022
const uint8_t max_recovery_attempts = 3;
10021023

1024+
// Validate inputs before acquiring mutex to reduce lock contention
1025+
if (__builtin_expect(!opus_buf || opus_size <= 0 || opus_size > max_packet_size, 0)) {
1026+
return -1;
1027+
}
1028+
10031029
if (__builtin_expect(atomic_load(&playback_stop_requested), 0)) {
10041030
return -1;
10051031
}
@@ -1008,12 +1034,7 @@ __attribute__((hot)) int jetkvm_audio_decode_write(void * __restrict__ opus_buf,
10081034

10091035
pthread_mutex_lock(&playback_mutex);
10101036

1011-
if (__builtin_expect(!playback_initialized || !pcm_playback_handle || !decoder || !opus_buf || opus_size <= 0, 0)) {
1012-
pthread_mutex_unlock(&playback_mutex);
1013-
return -1;
1014-
}
1015-
1016-
if (opus_size > max_packet_size) {
1037+
if (__builtin_expect(!playback_initialized || !pcm_playback_handle || !decoder, 0)) {
10171038
pthread_mutex_unlock(&playback_mutex);
10181039
return -1;
10191040
}
@@ -1027,12 +1048,26 @@ __attribute__((hot)) int jetkvm_audio_decode_write(void * __restrict__ opus_buf,
10271048
pcm_frames = opus_decode(dec, in, opus_size, pcm_buffer, opus_frame_size, 0);
10281049

10291050
if (__builtin_expect(pcm_frames < 0, 0)) {
1051+
// Initial decode failed, try Forward Error Correction from previous packets
1052+
fprintf(stderr, "WARN: playback: Opus decode failed (%d), attempting FEC recovery\n", pcm_frames);
1053+
fflush(stderr);
1054+
10301055
pcm_frames = opus_decode(dec, NULL, 0, pcm_buffer, opus_frame_size, 1);
10311056

10321057
if (pcm_frames < 0) {
1058+
fprintf(stderr, "ERROR: playback: FEC recovery also failed (%d), dropping frame\n", pcm_frames);
1059+
fflush(stderr);
10331060
pthread_mutex_unlock(&playback_mutex);
10341061
return -1;
10351062
}
1063+
1064+
if (pcm_frames > 0) {
1065+
fprintf(stdout, "INFO: playback: FEC recovered %d frames\n", pcm_frames);
1066+
fflush(stdout);
1067+
} else {
1068+
fprintf(stderr, "WARN: playback: FEC returned 0 frames (silence)\n");
1069+
fflush(stderr);
1070+
}
10361071
}
10371072

10381073
retry_write:

internal/audio/relay.go

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,8 +71,10 @@ func (r *OutputRelay) relayLoop() {
7171
defer close(r.stopped)
7272

7373
const maxRetries = 10
74+
const maxConsecutiveWriteFailures = 50 // Allow some WebRTC write failures before reconnecting
7475
retryDelay := 1 * time.Second
7576
consecutiveFailures := 0
77+
consecutiveWriteFailures := 0
7678

7779
for r.running.Load() {
7880
// Connect if not connected
@@ -108,7 +110,7 @@ func (r *OutputRelay) relayLoop() {
108110
continue
109111
}
110112

111-
// Reset retry state on success
113+
// Reset retry state on successful read
112114
consecutiveFailures = 0
113115
retryDelay = 1 * time.Second
114116

@@ -117,9 +119,28 @@ func (r *OutputRelay) relayLoop() {
117119
r.sample.Data = payload
118120
if err := r.audioTrack.WriteSample(r.sample); err != nil {
119121
r.framesDropped.Add(1)
120-
r.logger.Warn().Err(err).Msg("Failed to write sample to WebRTC")
122+
consecutiveWriteFailures++
123+
124+
// Log warning on first failure and every 10th failure
125+
if consecutiveWriteFailures == 1 || consecutiveWriteFailures%10 == 0 {
126+
r.logger.Warn().
127+
Err(err).
128+
Int("consecutive_failures", consecutiveWriteFailures).
129+
Msg("Failed to write sample to WebRTC")
130+
}
131+
132+
// If too many consecutive write failures, reconnect source
133+
if consecutiveWriteFailures >= maxConsecutiveWriteFailures {
134+
r.logger.Error().
135+
Int("failures", consecutiveWriteFailures).
136+
Msg("Too many consecutive WebRTC write failures, reconnecting source")
137+
(*r.source).Disconnect()
138+
consecutiveWriteFailures = 0
139+
consecutiveFailures = 0
140+
}
121141
} else {
122142
r.framesRelayed.Add(1)
143+
consecutiveWriteFailures = 0 // Reset on successful write
123144
}
124145
}
125146
}

0 commit comments

Comments
 (0)