Add support for multi codec negotiation #741
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for multi-codec negotiation in the WebRTC media engine to address issue #737. The change allows the media engine to negotiate multiple codecs instead of stopping after the first successful negotiation.
Key Changes:
- Added a new
negotiate_multi_codecsfield to control whether multiple codecs can be negotiated - Modified the codec negotiation logic to continue processing when multi-codec negotiation is enabled
- Added comprehensive test coverage for both single and multi-codec negotiation scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| webrtc/src/api/media_engine/mod.rs | Added multi-codec negotiation field, getter/setter methods, and updated negotiation logic |
| webrtc/src/api/media_engine/media_engine_test.rs | Added test cases to verify multi-codec negotiation functionality |
Comments suppressed due to low confidence (1)
webrtc/src/api/media_engine/mod.rs:1
- The methods
set_multi_codec_negotiationandmulti_codec_negotiationhave inconsistent visibility - they are private (fn) but should likely be public (pub fn) to allow external configuration of this feature.
#[cfg(test)]
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| let typ = if (!self.negotiated_audio.load(Ordering::SeqCst) | ||
| || self.negotiate_multi_codecs.load(Ordering::SeqCst)) | ||
| && media.media_name.media.to_lowercase() == "audio" | ||
| { | ||
| self.negotiated_audio.store(true, Ordering::SeqCst); | ||
| RTPCodecType::Audio | ||
| } else if !self.negotiated_video.load(Ordering::SeqCst) | ||
| } else if (!self.negotiated_video.load(Ordering::SeqCst) | ||
| || self.negotiate_multi_codecs.load(Ordering::SeqCst)) |
There was a problem hiding this comment.
[nitpick] The parentheses around the first condition are unnecessary and reduce readability. The logical precedence of ! and || operators makes these parentheses redundant.
Copilot uses AI. Check for mistakes.
| } else if (!self.negotiated_video.load(Ordering::SeqCst) | ||
| || self.negotiate_multi_codecs.load(Ordering::SeqCst)) |
There was a problem hiding this comment.
[nitpick] The parentheses around the first condition are unnecessary and reduce readability. The logical precedence of ! and || operators makes these parentheses redundant.
Copilot uses AI. Check for mistakes.
rainliu
force-pushed
the
multi-codec
branch
from
0f971d9 to
3479ccf
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // If we have attempted to negotiate a codec type yet. | ||
| pub(crate) negotiated_video: AtomicBool, | ||
| pub(crate) negotiated_audio: AtomicBool, | ||
| pub(crate) negotiate_multi_codecs: AtomicBool, |
There was a problem hiding this comment.
The new field negotiate_multi_codecs is not initialized in the default constructor. This could lead to undefined behavior. Consider adding initialization in the Default implementation or constructor.
Copilot uses AI. Check for mistakes.
address #737