feat: Add audio track for hang-c library #729
Conversation
WalkthroughThis change replaces Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
rs/hang-c/src/lib.rs (3)
66-68: Lock poisoning silently ignored.If the lock is poisoned, the thread handle will be leaked. While lock poisoning is rare, consider logging the error or using
.expect()with a descriptive message for debugging.Apply this diff to add error handling:
- if let Ok(mut guard) = HANDLE.lock() { - *guard = Some(handle); - } + *HANDLE.lock().expect("HANDLE lock poisoned") = Some(handle);
74-81: Improved shutdown sequence.The addition of thread join and IMPORT clearing provides cleaner shutdown behavior. However, consider that write operations (
hang_write_video_packet_from_c,hang_write_audio_packet_from_c) might still execute if they acquired locks beforeIMPORTis cleared.For more robust error handling, consider:
- if let Ok(mut guard) = HANDLE.lock() { - if let Some(handle) = guard.take() { - let _ = handle.join(); - } - } - if let Ok(mut guard) = IMPORT.lock() { - *guard = None; - } + if let Ok(mut guard) = HANDLE.lock() { + if let Some(handle) = guard.take() { + let _ = handle.join(); + } + } else { + eprintln!("Warning: HANDLE lock poisoned during shutdown"); + } + + if let Ok(mut guard) = IMPORT.lock() { + *guard = None; + } else { + eprintln!("Warning: IMPORT lock poisoned during shutdown"); + }
206-223: Audio track initialization looks good.The audio track integration follows the same pattern as video and correctly adds it to the catalog. The audio priority of 2 (vs. video priority of 1) prioritizes audio delivery.
Consider using constants or an enum for track indices instead of hardcoding
0and1:const VIDEO_TRACK_ID: u32 = 0; const AUDIO_TRACK_ID: u32 = 1;This would make the code more maintainable and reduce the risk of index mismatches between
init()and the write methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rs/hang-c/src/lib.rs(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Rust tests should be integrated within source files
Files:
rs/hang-c/src/lib.rs
rs/**/*.{rs,toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Rust workspace configured in
rs/Cargo.tomlfor Rust development
Files:
rs/hang-c/src/lib.rs
🔇 Additional comments (8)
rs/hang-c/src/lib.rs (8)
5-5: LGTM!The additional imports for audio support (
Audio,AudioConfig,AAC) are appropriate and used throughout the file.
96-101: LGTM!The lock-based access is consistent with the refactor and correctly uses
as_mut()to obtain a mutable reference for writing video frames.
104-122: LGTM!The new audio packet function is well-structured with proper safety documentation, input validation, and consistent locking patterns matching the video counterpart.
137-139: LGTM!The lock-based assignment is consistent with the synchronization refactor.
187-187: Priority adjustments prioritize audio over video.The priority changes (video: 2→1, audio: 2) ensure audio is delivered with higher priority than video. This is a sensible choice for media streaming, as audio interruptions are typically more disruptive to user experience than video frame drops.
Also applies to: 198-198, 209-209, 220-220
299-319: LGTM!The audio frame writing implementation correctly:
- Includes safety documentation
- Retrieves the audio track (index 1)
- Copies data to prevent use-after-free
- Sets
keyframe: false(appropriate for audio frames)- Mirrors the video frame writing pattern
16-17: I'll help you verify the race condition concern in this review. Let me start by examining the current code and understanding the synchronization mechanisms.
<function_calls>
#!/bin/bashRead the file to see lines 16-17 and surrounding context
cat -n rs/hang-c/src/lib.rs | head -50
</function_calls>Let me also check the git history to understand if this was changed from OnceLock:
<function_calls>
#!/bin/bashCheck recent git history on this file
git log --oneline -10 -- rs/hang-c/src/lib.rs
</function_calls>Now let me examine the implementation of hang_start_from_c() and the RUNNING flag:
<function_calls>
#!/bin/bashSearch for hang_start_from_c and RUNNING flag
echo "=== Searching for hang_start_from_c implementation ==="
rg -n 'hang_start_from_c' rs/hang-c/src/lib.rs -A 15echo -e "\n=== Searching for RUNNING flag usage ==="
rg -n 'RUNNING' rs/hang-c/src/lib.rs -B 2 -A 2
</function_calls>Let me also check the complete file to understand the full context:
<function_calls>
#!/bin/bashCheck file size and read the entire file
wc -l rs/hang-c/src/lib.rs
echo "---"
cat -n rs/hang-c/src/lib.rs
</function_calls>Let me check the git diff to see what changed from previous version:
<function_calls>
#!/bin/bashShow the recent diff or changes to understand what was modified
git diff HEAD
1 rs/hang-c/src/lib.rs 2>/dev/null || echo "Could not get diff from HEAD1"echo -e "\n=== Show current HEAD commit ==="
git log -1 --oneline rs/hang-c/src/lib.rs
</function_calls>
252-267: AAC profile 2 is correctly specified as AAC-LC. Per ISO/IEC 14496-3, audio object type 2 corresponds to AAC Low Complexity (AAC-LC), which is the standard baseline profile for general-purpose audio streaming and broadcast. The audio configuration is appropriate: 48 kHz sample rate, stereo (2 channels), and 128 kbps bitrate are all reasonable values for AAC-LC encoding.
|
Can you run |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
rs/hang-c/src/lib.rs (3)
12-17: GlobalMutex<Option<...>>usage can silently ignore poisoning and has slightly fuzzy restart semanticsUsing
Mutex<Option<ImportJoy>>/Mutex<Option<JoinHandle<()>>>is fine, but the repeatedif let Ok(mut guard) = ...pattern means any panic while the mutex is held will poison it and make all laterhang_write_*/hang_stop_from_ccalls effectively no‑ops (lock returnsErr, branch is skipped, and IMPORT/HANDLE never get updated or cleared). Also, callinghang_start_from_cmultiple times before a matching stop drops the previousJoinHandlewithout joining it, so only the most recent client is tracked explicitly. If you expect panics/restarts in real usage, considermatching onOk/Err(poisoned)and always updating the innerOptionviapoisoned.into_inner(), and/or guardinghang_start_from_cto be idempotent while a client is already running (e.g., checkRUNNING/existingHANDLEand early‑return).Also applies to: 66-68, 74-81, 96-101, 116-121, 137-139
104-122: Audio FFI path and track wiring look good; consider de‑magicking track IDs and double‑check priority semanticsThe new audio flow (
hang_write_audio_packet_from_c→ImportJoy::write_audio_frameplusinit_audiowiring) mirrors the video path nicely: pointers are validated, data is copied into ownedBytes, and timestamps useTimestamp::from_micros(dts)consistently. Two small maintainability/nastiness points:
- The
self.trackskeys0(video) and1(audio) are magic numbers. Defining shared constants and using them in bothinit*andwrite_*would make future changes safer:+const VIDEO_TRACK_ID: u32 = 0; +const AUDIO_TRACK_ID: u32 = 1; @@ - self.tracks.insert(0, track_produce.producer.into()); + self.tracks.insert(VIDEO_TRACK_ID, track_produce.producer.into()); @@ - self.tracks.insert(1, track_produce.producer.into()); + self.tracks.insert(AUDIO_TRACK_ID, track_produce.producer.into()); @@ - let Some(track) = self.tracks.get_mut(&0) else { + let Some(track) = self.tracks.get_mut(&VIDEO_TRACK_ID) else { @@ - let Some(track) = self.tracks.get_mut(&1) else { + let Some(track) = self.tracks.get_mut(&AUDIO_TRACK_ID) else {
- Audio gets
priority: 2while video ispriority: 1on both theTrackand{Video, Audio}configs. If “higher number = higher preference” is the intended contract for thehangcatalog/player, this is fine but might be worth a short comment to document that audio is intentionally preferred over video for selection.Also applies to: 179-223, 252-264, 295-315
252-264: Add minimal in‑source tests for new audio behaviorThe new audio initialization and write path (
init_audio/write_audio_frame) are untested. A small#[cfg(test)]module that constructs anImportJoy, callsinit(), and exerciseswrite_audio_frame(asserting that frames are accepted for the audio track and timestamp units are respected) would help catch regressions in track IDs, codec config, or timestamp handling without relying only on C‑side integration tests. As per coding guidelines, Rust tests should be integrated within source files.Also applies to: 295-315
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rs/hang-c/src/lib.rs(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Rust tests should be integrated within source files
Files:
rs/hang-c/src/lib.rs
rs/**/*.{rs,toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Rust workspace configured in
rs/Cargo.tomlfor Rust development
Files:
rs/hang-c/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
2 participants
No description provided.