Add Opus decoder #811
Add Opus decoder #811
Conversation
WalkthroughAdds Opus decoding support across the import pipeline. A new module Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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: 2
🧹 Nitpick comments (2)
rs/hang/src/import/opus.rs (2)
43-65: Consider guarding against double initialization.Calling
initialize()multiple times would create duplicate tracks in the broadcast. Consider adding a guard at the start of the method.🔎 Proposed fix
pub fn initialize<T: Buf>(&mut self, buf: &mut T) -> anyhow::Result<()> { + anyhow::ensure!(self.track.is_none(), "already initialized"); + // Parse OpusHead (https://datatracker.ietf.org/doc/html/rfc7845#section-5.1)
1-112: Consider adding unit tests for the Opus decoder.As per coding guidelines, Rust crates should have tests integrated within source files using inline test modules. Consider adding tests for:
initialize()with valid OpusHead datainitialize()with truncated/invalid datadecode()behavior when not initializeddecode()with valid frames🔎 Example test module structure
#[cfg(test)] mod tests { use super::*; use bytes::Bytes; fn sample_opus_head() -> Bytes { // OpusHead: signature(8) + version(1) + channels(1) + pre_skip(2) + sample_rate(4) + gain(2) + mapping(1) let mut buf = Vec::new(); buf.extend_from_slice(b"OpusHead"); buf.push(1); // version buf.push(2); // channels buf.extend_from_slice(&0u16.to_le_bytes()); // pre_skip buf.extend_from_slice(&48000u32.to_le_bytes()); // sample_rate buf.extend_from_slice(&0i16.to_le_bytes()); // output_gain buf.push(0); // channel_mapping_family Bytes::from(buf) } #[test] fn test_initialize_parses_opus_head() { // TODO: Implement with mock BroadcastProducer } }Based on coding guidelines, tests should be integrated within source files.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rs/hang/src/import/decoder.rsrs/hang/src/import/mod.rsrs/hang/src/import/opus.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
In Rust crates, tests should be integrated within source files using inline test modules
Files:
rs/hang/src/import/opus.rsrs/hang/src/import/mod.rsrs/hang/src/import/decoder.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
🔇 Additional comments (8)
rs/hang/src/import/mod.rs (1)
6-6: LGTM!The module declaration and re-export follow the established pattern used by other decoders in this file.
Also applies to: 13-13
rs/hang/src/import/decoder.rs (4)
5-5: LGTM!The import and
DecoderFormat::Opusvariant are correctly added following the existing pattern.Also applies to: 17-18
33-33: LGTM!The
FromStrandDisplayimplementations are symmetric and follow the existing pattern.Also applies to: 45-45
57-57: LGTM!The
DecoderKind::Opusvariant and instantiation inDecoder::newcorrectly follow the pattern established byAac.Also applies to: 75-75
92-92: LGTM!All decoder lifecycle methods correctly handle the Opus variant. The stream decoding bail aligns with AAC's behavior, which is appropriate since Opus frames require external framing.
Also applies to: 117-119, 144-144, 156-156
rs/hang/src/import/opus.rs (3)
70-89: LGTM!The decode method correctly drains the buffer into a
BufListand creates a frame. Marking all Opus frames as keyframes is appropriate since Opus packets are independently decodable.
91-102: LGTM!The
is_initializedcheck andptshelper follow a sensible pattern. The timestamp derivation from wall clock time provides reasonable fallback behavior.
105-111: LGTM!The
Dropimplementation properly cleans up by removing the audio track from the catalog, ensuring resources are released.
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Just gotta fix buf.chunk() then LGTM.
In practice it makes zero difference, as OBS gives us a contiguous buffer, but bytes::Buf doesn't need to be contiguous.
| // - Reads sample rate | ||
| // - Ignores pre-skip, gain, channel mapping for now | ||
|
|
||
| anyhow::ensure!(buf.remaining() >= 19, "OpusHead must be at least 19 bytes"); |
There was a problem hiding this comment.
Another option are thetry_get_xxx methods in bytes::Buf. I really don't like how the default behavior is to panic; it's not like it's faster anyway because it still performs the range check.
|
Re the slow group errors, I'm almost done with adjustable latency so we can support multiple audio frames in flight. I don't really know what OBS is doing (unnecessary buffering?) but AAC has the same problem. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
rs/hang/src/import/opus.rs (2)
35-38: Considertry_get_*methods for defensive parsing.While the bounds check at line 30 ensures safety, using
try_get_u8(),try_get_u32_le()etc. would align with kixelated's feedback about avoiding panic-by-default methods and make the code more defensive against future refactoring.Based on past review comments from kixelated.
78-80: Verify chunk-copying performance aligns with comment intent.The comment mentions "potentially avoiding allocations," but
copy_to_bytesallocates a newBytesfor each chunk. If zero-copy is desired, consider whetherBufListsupports taking ownership of existing chunks directly. Otherwise, the current approach is standard for consolidating fragmented buffers.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rs/hang/src/import/opus.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
In Rust crates, tests should be integrated within source files using inline test modules
Files:
rs/hang/src/import/opus.rs
🧬 Code graph analysis (1)
rs/hang/src/import/opus.rs (2)
rs/hang/src/import/decoder.rs (3)
new(70-79)initialize(87-98)is_initialized(151-158)rs/hang/src/model/timestamp.rs (1)
from_micros(40-46)
⏰ 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
🔇 Additional comments (4)
rs/hang/src/import/opus.rs (4)
1-12: Clean structure following established patterns.The imports and struct definition are well-organized. The use of
Option<tokio::time::Instant>for timestamp generation is appropriate.
30-33: Signature and bounds checks correctly implemented.The bounds check and signature verification using
const OPUS_HEADwithget_u64()properly address the fragmented buffer issue raised in previous reviews.
107-114: Proper resource cleanup in Drop.The Drop implementation correctly removes the audio track from the catalog and logs the teardown, ensuring no resource leaks.
84-84: Marking all Opus frames as keyframes is appropriate and aligns with Frame semantics.Opus frames are audio data that are independently decodable, which matches the Frame definition: "a keyframe (can be decoded independently)." This design creates an entry point for new subscribers at each frame boundary, fulfilling the Frame's stated purpose of supporting new subscribers with keyframe-based group management.
| fn pts(&mut self, hint: Option<hang::Timestamp>) -> anyhow::Result<hang::Timestamp> { | ||
| if let Some(pts) = hint { | ||
| return Ok(pts); | ||
| } | ||
|
|
||
| let zero = self.zero.get_or_insert_with(tokio::time::Instant::now); | ||
| Ok(hang::Timestamp::from_micros(zero.elapsed().as_micros() as u64)?) | ||
| } |
There was a problem hiding this comment.
Timestamp truncation could silently wrap after ~584k years.
Line 103 casts as_micros() (u128) to u64 before calling from_micros, which silently truncates overflow. While not realistic in practice, using .try_into()? would properly propagate overflow errors.
🔎 Suggested fix
fn pts(&mut self, hint: Option<hang::Timestamp>) -> anyhow::Result<hang::Timestamp> {
if let Some(pts) = hint {
return Ok(pts);
}
let zero = self.zero.get_or_insert_with(tokio::time::Instant::now);
- Ok(hang::Timestamp::from_micros(zero.elapsed().as_micros() as u64)?)
+ let micros: u64 = zero.elapsed().as_micros().try_into()
+ .context("timestamp overflow")?;
+ Ok(hang::Timestamp::from_micros(micros)?)
}🤖 Prompt for AI Agents
In rs/hang/src/import/opus.rs around lines 97 to 104, the code casts
zero.elapsed().as_micros() (u128) to u64 using as which silently truncates on
overflow; replace the direct cast with a fallible conversion (e.g.
.as_micros().try_into()?), then pass the resulting u64 into
hang::Timestamp::from_micros so any overflow becomes a propagated error rather
than silent truncation.
2 participants
This PR adds Opus support in the hang library.
The Opus decoder is largely copied from the AAC decoder. It parses the OpusHead instead of the AudioSpecificConfig.
How to test:
opusas an audio encoder option: Add Opus audio encoder obs#18This is working well for me locally on Ubuntu 24. The only issue I've seen is that occasionally I get these logs spamming in the web player. And some crackling from underflow. Any ideas on what might be going on here?
The plugin logs seem to show a similar off-by-one issue: