From 49af4ccee8587a233988c65dbcdb00b2e9212200 Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Tue, 7 Apr 2026 12:44:23 +0000 Subject: [PATCH 1/7] refactor: formalize C ABI codec registry via as_c_name/from_c_name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Centralizes codec name ↔ enum conversion in AudioCodec and VideoCodec, eliminating hardcoded string matching duplicated across 6 locations in the plugin SDK (conversions.rs, processor macro, source macro × input/output). Key changes: - Add AudioCodec::as_c_name()/from_c_name() and VideoCodec equivalents as the single source of truth for C ABI codec name strings. - Fix EncodedVideo to carry codec info via custom_type_id (same pattern as EncodedAudio). Previously mapped to PacketType::Binary with a TODO. Null custom_type_id falls back to Binary for backward compat. - Replace 4 duplicated match blocks in native_plugin_entry! and native_source_plugin_entry! macros with as_c_name() calls. - Add EncodedVideo custom_type_id handling to both macros. - Document the codec extensibility pattern in CPacketTypeInfo. - Add roundtrip tests for all codec name conversions and C ABI encoding. Adding a new codec now requires only two changes: 1. Add the variant to AudioCodec/VideoCodec 2. Add its name in as_c_name() and from_c_name() No API version bump needed — CPacketTypeInfo struct layout unchanged. BREAKING CHANGE: EncodedVideo pin declarations now carry codec info via custom_type_id instead of mapping to Binary. Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- crates/core/src/types.rs | 85 ++++++++++++ sdks/plugin-sdk/native/src/conversions.rs | 149 ++++++++++++++++++---- sdks/plugin-sdk/native/src/lib.rs | 54 ++++---- sdks/plugin-sdk/native/src/types.rs | 23 +++- 4 files changed, 257 insertions(+), 54 deletions(-) diff --git a/crates/core/src/types.rs b/crates/core/src/types.rs index d91d2f6c..685aef97 100644 --- a/crates/core/src/types.rs +++ b/crates/core/src/types.rs @@ -81,6 +81,29 @@ impl AudioCodec { Self::Aac => 21_333, } } + + /// Canonical lowercase name used in the C ABI (`custom_type_id`). + /// + /// Adding a new `AudioCodec` variant? Add its name here and in + /// [`Self::from_c_name`] — that's the **only** place codec-name + /// strings need to live. + pub const fn as_c_name(self) -> &'static str { + match self { + Self::Opus => "opus", + Self::Aac => "aac", + } + } + + /// Parse a C ABI codec name back to an `AudioCodec`. + /// + /// Accepts the canonical lowercase names produced by [`Self::as_c_name`]. + pub fn from_c_name(name: &str) -> Result { + match name { + "opus" => Ok(Self::Opus), + "aac" => Ok(Self::Aac), + other => Err(format!("unknown audio codec name: {other:?}")), + } + } } /// Supported encoded video codecs. @@ -99,6 +122,33 @@ pub enum VideoCodec { Av1, } +impl VideoCodec { + /// Canonical lowercase name used in the C ABI (`custom_type_id`). + /// + /// Adding a new `VideoCodec` variant? Add its name here and in + /// [`Self::from_c_name`] — that's the **only** place codec-name + /// strings need to live. + pub const fn as_c_name(self) -> &'static str { + match self { + Self::Vp9 => "vp9", + Self::H264 => "h264", + Self::Av1 => "av1", + } + } + + /// Parse a C ABI codec name back to a `VideoCodec`. + /// + /// Accepts the canonical lowercase names produced by [`Self::as_c_name`]. + pub fn from_c_name(name: &str) -> Result { + match name { + "vp9" => Ok(Self::Vp9), + "h264" | "avc" | "avc1" => Ok(Self::H264), + "av1" => Ok(Self::Av1), + other => Err(format!("unknown video codec name: {other:?}")), + } + } +} + /// Bitstream format hints for video codecs (primarily H264). #[derive(Debug, PartialEq, Eq, Clone, Copy, Serialize, Deserialize, JsonSchema, TS)] #[ts(export)] @@ -897,4 +947,39 @@ mod tests { assert_eq!(pool.stats().buckets[0].available, 1); } + + // ── Codec C-name roundtrip tests ─────────────────────────────────── + + #[test] + fn audio_codec_c_name_roundtrip() { + for codec in [AudioCodec::Opus, AudioCodec::Aac] { + let name = codec.as_c_name(); + let parsed = AudioCodec::from_c_name(name) + .unwrap_or_else(|e| panic!("roundtrip failed for {codec:?}: {e}")); + assert_eq!(codec, parsed, "roundtrip mismatch for {name:?}"); + } + } + + #[test] + fn video_codec_c_name_roundtrip() { + for codec in [VideoCodec::Vp9, VideoCodec::H264, VideoCodec::Av1] { + let name = codec.as_c_name(); + let parsed = VideoCodec::from_c_name(name) + .unwrap_or_else(|e| panic!("roundtrip failed for {codec:?}: {e}")); + assert_eq!(codec, parsed, "roundtrip mismatch for {name:?}"); + } + } + + #[test] + fn video_codec_from_c_name_aliases() { + // H264 should accept common aliases from container formats. + assert_eq!(VideoCodec::from_c_name("avc").unwrap(), VideoCodec::H264); + assert_eq!(VideoCodec::from_c_name("avc1").unwrap(), VideoCodec::H264); + } + + #[test] + fn codec_from_c_name_unknown_errors() { + assert!(AudioCodec::from_c_name("mp3").is_err()); + assert!(VideoCodec::from_c_name("hevc").is_err()); + } } diff --git a/sdks/plugin-sdk/native/src/conversions.rs b/sdks/plugin-sdk/native/src/conversions.rs index cee895aa..8ef00450 100644 --- a/sdks/plugin-sdk/native/src/conversions.rs +++ b/sdks/plugin-sdk/native/src/conversions.rs @@ -18,8 +18,8 @@ use std::sync::Arc; use streamkit_core::frame_pool::{PooledSamples, PooledVideoData}; use streamkit_core::types::{ AudioCodec, AudioFormat, AudioFrame, CustomEncoding, CustomPacketData, EncodedAudioFormat, - Packet, PacketMetadata, PacketType, PixelFormat, RawVideoFormat, SampleFormat, - TranscriptionData, VideoFrame, + EncodedVideoFormat, Packet, PacketMetadata, PacketType, PixelFormat, RawVideoFormat, + SampleFormat, TranscriptionData, VideoCodec, VideoFrame, }; /// Convert C packet type info to Rust PacketType @@ -62,12 +62,24 @@ pub fn packet_type_from_c(cpt_info: CPacketTypeInfo) -> Result { - // TODO: Add CVideoCodec enum to carry codec through the C ABI. - // Until then, EncodedVideo is accepted as a discriminant for pin - // declarations but the codec field is meaningless. Using Binary - // as the packet-level representation (see `packet_from_c`) avoids - // silently mislabelling the codec. - Ok(PacketType::Binary) + // The codec name is carried in `custom_type_id` (same pattern as + // EncodedAudio). Null `custom_type_id` falls back to Binary for + // backward compat with plugins compiled before codec strings were + // added to EncodedVideo. + if cpt_info.custom_type_id.is_null() { + Ok(PacketType::Binary) + } else { + let name = unsafe { c_str_to_string(cpt_info.custom_type_id) }?; + let codec = VideoCodec::from_c_name(&name) + .map_err(|_| format!("Unknown EncodedVideo codec name: {name:?}"))?; + Ok(PacketType::EncodedVideo(EncodedVideoFormat { + codec, + bitstream_format: None, + codec_private: None, + profile: None, + level: None, + })) + } }, CPacketType::EncodedAudio => { // The codec name is carried in `custom_type_id` to avoid changing @@ -77,11 +89,8 @@ pub fn packet_type_from_c(cpt_info: CPacketTypeInfo) -> Result AudioCodec::Opus, - "aac" => AudioCodec::Aac, - other => return Err(format!("Unknown EncodedAudio codec name: {other:?}")), - } + AudioCodec::from_c_name(&name) + .map_err(|_| format!("Unknown EncodedAudio codec name: {name:?}"))? }; Ok(PacketType::EncodedAudio(EncodedAudioFormat { codec, codec_private: None })) }, @@ -226,7 +235,9 @@ pub fn packet_type_to_c(pt: &PacketType) -> (CPacketTypeInfo, CPacketTypeOwned) } else { // Carry the codec name in `custom_type_id` (reusing the // existing pointer field to avoid changing the struct layout). - let codec_name: &'static [u8] = match format.codec { + // Static null-terminated byte strings for the C ABI; the + // canonical names match AudioCodec::as_c_name(). + let codec_cstr: &'static [u8] = match format.codec { AudioCodec::Opus => b"opus\0", AudioCodec::Aac => b"aac\0", _ => b"unknown\0", @@ -235,7 +246,7 @@ pub fn packet_type_to_c(pt: &PacketType) -> (CPacketTypeInfo, CPacketTypeOwned) CPacketTypeInfo { type_discriminant: CPacketType::EncodedAudio, audio_format: std::ptr::null(), - custom_type_id: codec_name.as_ptr().cast::(), + custom_type_id: codec_cstr.as_ptr().cast::(), raw_video_format: std::ptr::null(), }, CPacketTypeOwned::None, @@ -282,15 +293,25 @@ pub fn packet_type_to_c(pt: &PacketType) -> (CPacketTypeInfo, CPacketTypeOwned) CPacketTypeOwned::Video(c_fmt), ) }, - PacketType::EncodedVideo(_) => ( - CPacketTypeInfo { - type_discriminant: CPacketType::EncodedVideo, - audio_format: std::ptr::null(), - custom_type_id: std::ptr::null(), - raw_video_format: std::ptr::null(), - }, - CPacketTypeOwned::None, - ), + PacketType::EncodedVideo(format) => { + // Carry the video codec name in `custom_type_id` (same pattern + // as EncodedAudio). + let codec_cstr: &'static [u8] = match format.codec { + VideoCodec::Vp9 => b"vp9\0", + VideoCodec::H264 => b"h264\0", + VideoCodec::Av1 => b"av1\0", + _ => b"unknown\0", + }; + ( + CPacketTypeInfo { + type_discriminant: CPacketType::EncodedVideo, + audio_format: std::ptr::null(), + custom_type_id: codec_cstr.as_ptr().cast::(), + raw_video_format: std::ptr::null(), + }, + CPacketTypeOwned::None, + ) + }, PacketType::Binary => ( CPacketTypeInfo { type_discriminant: CPacketType::Binary, @@ -990,4 +1011,84 @@ mod tests { assert_eq!(repr.packet.packet_type, CPacketType::Binary, "should remain Binary"); assert_eq!(repr.packet.len, payload.len()); } + + // ── EncodedVideo codec roundtrip tests ───────────────────────────── + + /// `packet_type_to_c` → `packet_type_from_c` must roundtrip all video + /// codecs through the `custom_type_id` string pointer. + #[test] + fn encoded_video_codec_roundtrip_via_c() { + use streamkit_core::types::{EncodedVideoFormat, VideoCodec}; + + for codec in [VideoCodec::Vp9, VideoCodec::H264, VideoCodec::Av1] { + let pt = PacketType::EncodedVideo(EncodedVideoFormat { + codec, + bitstream_format: None, + codec_private: None, + profile: None, + level: None, + }); + + let (info, _owned) = packet_type_to_c(&pt); + assert_eq!( + info.type_discriminant, + CPacketType::EncodedVideo, + "discriminant mismatch for {codec:?}" + ); + assert!(!info.custom_type_id.is_null(), "custom_type_id should be set for {codec:?}"); + + let roundtripped = packet_type_from_c(info) + .unwrap_or_else(|e| panic!("roundtrip failed for {codec:?}: {e}")); + + match roundtripped { + PacketType::EncodedVideo(fmt) => { + assert_eq!(fmt.codec, codec, "codec mismatch after roundtrip"); + }, + other => panic!("expected EncodedVideo, got {other:?}"), + } + } + } + + /// `EncodedVideo` with null `custom_type_id` falls back to `Binary` + /// (backward compat with pre-codec-string plugins). + #[test] + fn encoded_video_null_codec_falls_back_to_binary() { + let info = CPacketTypeInfo { + type_discriminant: CPacketType::EncodedVideo, + audio_format: std::ptr::null(), + custom_type_id: std::ptr::null(), + raw_video_format: std::ptr::null(), + }; + let pt = packet_type_from_c(info) + .unwrap_or_else(|e| panic!("null custom_type_id should fall back to Binary: {e}")); + assert_eq!(pt, PacketType::Binary); + } + + /// `EncodedAudio` roundtrips correctly through `custom_type_id`. + #[test] + fn encoded_audio_codec_roundtrip_via_c() { + for codec in [AudioCodec::Opus, AudioCodec::Aac] { + let pt = PacketType::EncodedAudio(EncodedAudioFormat { codec, codec_private: None }); + + let (info, _owned) = packet_type_to_c(&pt); + + // Opus without codec_private uses the legacy OpusAudio discriminant. + if codec == AudioCodec::Opus { + assert_eq!(info.type_discriminant, CPacketType::OpusAudio); + } else { + assert_eq!(info.type_discriminant, CPacketType::EncodedAudio); + assert!(!info.custom_type_id.is_null()); + } + + let roundtripped = packet_type_from_c(info) + .unwrap_or_else(|e| panic!("roundtrip failed for {codec:?}: {e}")); + + match roundtripped { + PacketType::EncodedAudio(fmt) => { + assert_eq!(fmt.codec, codec, "codec mismatch after roundtrip"); + }, + other => panic!("expected EncodedAudio, got {other:?}"), + } + } + } } diff --git a/sdks/plugin-sdk/native/src/lib.rs b/sdks/plugin-sdk/native/src/lib.rs index 0f16b6d6..0b8cd1c3 100644 --- a/sdks/plugin-sdk/native/src/lib.rs +++ b/sdks/plugin-sdk/native/src/lib.rs @@ -907,12 +907,12 @@ macro_rules! native_plugin_entry { )) } $crate::streamkit_core::types::PacketType::EncodedAudio(format) => { - let codec_name = match format.codec { - $crate::streamkit_core::types::AudioCodec::Opus => "opus", - $crate::streamkit_core::types::AudioCodec::Aac => "aac", - _ => "unknown", - }; - Some(std::ffi::CString::new(codec_name).expect( + Some(std::ffi::CString::new(format.codec.as_c_name()).expect( + "Codec name should not contain null bytes", + )) + } + $crate::streamkit_core::types::PacketType::EncodedVideo(format) => { + Some(std::ffi::CString::new(format.codec.as_c_name()).expect( "Codec name should not contain null bytes", )) } @@ -1034,14 +1034,12 @@ macro_rules! native_plugin_entry { )) } $crate::streamkit_core::types::PacketType::EncodedAudio(format) => { - // Carry the codec name in custom_type_id for - // non-Opus EncodedAudio variants (e.g. AAC). - let codec_name = match format.codec { - $crate::streamkit_core::types::AudioCodec::Opus => "opus", - $crate::streamkit_core::types::AudioCodec::Aac => "aac", - _ => "unknown", - }; - Some(std::ffi::CString::new(codec_name).expect( + Some(std::ffi::CString::new(format.codec.as_c_name()).expect( + "Codec name should not contain null bytes", + )) + } + $crate::streamkit_core::types::PacketType::EncodedVideo(format) => { + Some(std::ffi::CString::new(format.codec.as_c_name()).expect( "Codec name should not contain null bytes", )) } @@ -1441,13 +1439,14 @@ macro_rules! native_source_plugin_entry { ) }, $crate::streamkit_core::types::PacketType::EncodedAudio(format) => { - let codec_name = match format.codec { - $crate::streamkit_core::types::AudioCodec::Opus => "opus", - $crate::streamkit_core::types::AudioCodec::Aac => "aac", - _ => "unknown", - }; Some( - std::ffi::CString::new(codec_name) + std::ffi::CString::new(format.codec.as_c_name()) + .expect("Codec name should not contain null bytes"), + ) + }, + $crate::streamkit_core::types::PacketType::EncodedVideo(format) => { + Some( + std::ffi::CString::new(format.codec.as_c_name()) .expect("Codec name should not contain null bytes"), ) }, @@ -1567,15 +1566,14 @@ macro_rules! native_source_plugin_entry { .expect("Custom type_id should not contain null bytes"), ), $crate::streamkit_core::types::PacketType::EncodedAudio(format) => { - // Carry the codec name in custom_type_id for - // non-Opus EncodedAudio variants (e.g. AAC). - let codec_name = match format.codec { - $crate::streamkit_core::types::AudioCodec::Opus => "opus", - $crate::streamkit_core::types::AudioCodec::Aac => "aac", - _ => "unknown", - }; Some( - std::ffi::CString::new(codec_name) + std::ffi::CString::new(format.codec.as_c_name()) + .expect("Codec name should not contain null bytes"), + ) + }, + $crate::streamkit_core::types::PacketType::EncodedVideo(format) => { + Some( + std::ffi::CString::new(format.codec.as_c_name()) .expect("Codec name should not contain null bytes"), ) }, diff --git a/sdks/plugin-sdk/native/src/types.rs b/sdks/plugin-sdk/native/src/types.rs index 7ed6966a..71f31856 100644 --- a/sdks/plugin-sdk/native/src/types.rs +++ b/sdks/plugin-sdk/native/src/types.rs @@ -150,6 +150,9 @@ pub enum CPacketType { Any = 6, Passthrough = 7, RawVideo = 8, + /// Encoded video with codec metadata. Uses `custom_type_id` in + /// [`CPacketTypeInfo`] to carry the codec name (e.g. `"vp9"`, `"h264"`). + /// Null `custom_type_id` falls back to `Binary` for backward compat. EncodedVideo = 9, /// Binary packet that preserves optional `content_type` and `metadata` /// across the plugin ↔ host boundary. Points to a [`CBinaryPacket`]. @@ -238,9 +241,22 @@ pub struct CCustomPacket { /// Exactly one of the optional pointers is non-null depending on /// `type_discriminant`: /// - `RawAudio` → `audio_format` -/// - `Custom` → `custom_type_id` +/// - `Custom` → `custom_type_id` (null-terminated type id string) /// - `RawVideo` → `raw_video_format` /// - `EncodedAudio` → `custom_type_id` (null-terminated codec name, e.g. `"aac"`) +/// - `EncodedVideo` → `custom_type_id` (null-terminated codec name, e.g. `"h264"`) +/// +/// ## Adding a new codec +/// +/// Codec names are the canonical lowercase strings defined by +/// [`AudioCodec::as_c_name()`] and [`VideoCodec::as_c_name()`] in +/// `streamkit-core`. To add a new codec: +/// +/// 1. Add the variant to `AudioCodec` or `VideoCodec`. +/// 2. Add its name in `as_c_name()` and `from_c_name()` — these are the +/// **only** places codec-name strings need to live. +/// 3. The SDK macros and conversion functions use these methods, so no +/// further changes are needed in the plugin SDK. /// /// # ABI stability /// @@ -256,7 +272,10 @@ pub struct CPacketTypeInfo { pub audio_format: *const CAudioFormat, /// For Custom: pointer to a null-terminated type id string. /// For EncodedAudio: pointer to a null-terminated codec name - /// (e.g. `"opus"`, `"aac"`). Otherwise null. + /// (e.g. `"opus"`, `"aac"`). + /// For EncodedVideo: pointer to a null-terminated codec name + /// (e.g. `"vp9"`, `"h264"`, `"av1"`). + /// Otherwise null. pub custom_type_id: *const c_char, /// For RawVideo: pointer to CRawVideoFormat, otherwise null. pub raw_video_format: *const CRawVideoFormat, From 4c53a4f2b4085e0b58fb4989903b4ee3b3b11a5c Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Tue, 7 Apr 2026 12:53:17 +0000 Subject: [PATCH 2/7] refactor: use CPacketTypeOwned::CodecName to eliminate duplicated codec strings Add CodecName(CString) variant to CPacketTypeOwned and a codec_name_to_cstring() helper so packet_type_to_c derives its null-terminated codec names from as_c_name() instead of maintaining parallel hardcoded byte literals. This makes the doc comment claim true: as_c_name()/from_c_name() are now genuinely the only places codec-name strings live. Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- sdks/plugin-sdk/native/src/conversions.rs | 49 +++++++++++++---------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/sdks/plugin-sdk/native/src/conversions.rs b/sdks/plugin-sdk/native/src/conversions.rs index 8ef00450..94b32cf8 100644 --- a/sdks/plugin-sdk/native/src/conversions.rs +++ b/sdks/plugin-sdk/native/src/conversions.rs @@ -178,6 +178,18 @@ pub const fn raw_video_format_from_c(cfmt: &CRawVideoFormat) -> RawVideoFormat { } } +/// Build a `CString` from a codec name returned by `as_c_name()`. +/// +/// Codec names are compile-time ASCII constants that never contain interior +/// null bytes, so `CString::new` cannot fail here. +fn codec_name_to_cstring(name: &str) -> CString { + // Safety-net: as_c_name() values are controlled constants ("opus", "aac", + // "vp9", etc.) that will never contain '\0'. If this somehow fails, the + // empty CString is a safe (if wrong) fallback that will be rejected by + // from_c_name() on the receiving side. + CString::new(name).unwrap_or_default() +} + /// Ancillary data kept alive alongside a `CPacketTypeInfo`. /// /// `packet_type_to_c` returns this alongside the info struct so that @@ -188,6 +200,10 @@ pub enum CPacketTypeOwned { None, Audio(CAudioFormat), Video(CRawVideoFormat), + /// Null-terminated codec name derived from `AudioCodec::as_c_name()` or + /// `VideoCodec::as_c_name()`. The `custom_type_id` pointer in the + /// accompanying `CPacketTypeInfo` points into this `CString`. + CodecName(CString), } /// Convert Rust PacketType to C representation. @@ -233,23 +249,18 @@ pub fn packet_type_to_c(pt: &PacketType) -> (CPacketTypeInfo, CPacketTypeOwned) CPacketTypeOwned::None, ) } else { - // Carry the codec name in `custom_type_id` (reusing the - // existing pointer field to avoid changing the struct layout). - // Static null-terminated byte strings for the C ABI; the - // canonical names match AudioCodec::as_c_name(). - let codec_cstr: &'static [u8] = match format.codec { - AudioCodec::Opus => b"opus\0", - AudioCodec::Aac => b"aac\0", - _ => b"unknown\0", - }; + // Derive the null-terminated codec name from as_c_name() so + // the canonical name lives in exactly one place. + let name = codec_name_to_cstring(format.codec.as_c_name()); + let ptr = name.as_ptr(); ( CPacketTypeInfo { type_discriminant: CPacketType::EncodedAudio, audio_format: std::ptr::null(), - custom_type_id: codec_cstr.as_ptr().cast::(), + custom_type_id: ptr, raw_video_format: std::ptr::null(), }, - CPacketTypeOwned::None, + CPacketTypeOwned::CodecName(name), ) } }, @@ -294,22 +305,18 @@ pub fn packet_type_to_c(pt: &PacketType) -> (CPacketTypeInfo, CPacketTypeOwned) ) }, PacketType::EncodedVideo(format) => { - // Carry the video codec name in `custom_type_id` (same pattern - // as EncodedAudio). - let codec_cstr: &'static [u8] = match format.codec { - VideoCodec::Vp9 => b"vp9\0", - VideoCodec::H264 => b"h264\0", - VideoCodec::Av1 => b"av1\0", - _ => b"unknown\0", - }; + // Derive the null-terminated codec name from as_c_name() so + // the canonical name lives in exactly one place. + let name = codec_name_to_cstring(format.codec.as_c_name()); + let ptr = name.as_ptr(); ( CPacketTypeInfo { type_discriminant: CPacketType::EncodedVideo, audio_format: std::ptr::null(), - custom_type_id: codec_cstr.as_ptr().cast::(), + custom_type_id: ptr, raw_video_format: std::ptr::null(), }, - CPacketTypeOwned::None, + CPacketTypeOwned::CodecName(name), ) }, PacketType::Binary => ( From 8a4b0740b68119262c9928ed67ca1bc3e4f06fe6 Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Tue, 7 Apr 2026 15:35:32 +0000 Subject: [PATCH 3/7] fix: address review feedback on C ABI codec registry - codec_name_to_cstring: use expect() instead of unwrap_or_default() to fail fast on programmer errors (matches lib.rs macro pattern) - Add tracing::warn! when EncodedVideo null custom_type_id falls back to Binary (aids debugging of new plugins vs backward compat) - Simplify map_err to forward original from_c_name() error messages - Add doc comments clarifying intentional field loss (bitstream_format, codec_private, profile, level) through C ABI pin-type declarations Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- sdks/plugin-sdk/native/src/conversions.rs | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/sdks/plugin-sdk/native/src/conversions.rs b/sdks/plugin-sdk/native/src/conversions.rs index 94b32cf8..41e0bf64 100644 --- a/sdks/plugin-sdk/native/src/conversions.rs +++ b/sdks/plugin-sdk/native/src/conversions.rs @@ -67,11 +67,18 @@ pub fn packet_type_from_c(cpt_info: CPacketTypeInfo) -> Result Result Ok(PacketType::Binary), @@ -182,12 +191,9 @@ pub const fn raw_video_format_from_c(cfmt: &CRawVideoFormat) -> RawVideoFormat { /// /// Codec names are compile-time ASCII constants that never contain interior /// null bytes, so `CString::new` cannot fail here. +#[allow(clippy::expect_used)] // as_c_name() returns controlled constants; null bytes are a programmer error fn codec_name_to_cstring(name: &str) -> CString { - // Safety-net: as_c_name() values are controlled constants ("opus", "aac", - // "vp9", etc.) that will never contain '\0'. If this somehow fails, the - // empty CString is a safe (if wrong) fallback that will be rejected by - // from_c_name() on the receiving side. - CString::new(name).unwrap_or_default() + CString::new(name).expect("codec name from as_c_name() must not contain null bytes") } /// Ancillary data kept alive alongside a `CPacketTypeInfo`. From 08462363eacf153306a246bafb4d32d08ec051fe Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Tue, 7 Apr 2026 15:35:48 +0000 Subject: [PATCH 4/7] fix: address review feedback on C ABI codec registry - codec_name_to_cstring: use expect() instead of unwrap_or_default() to fail fast on programmer errors (matches lib.rs macro pattern) - Add tracing::warn! when EncodedVideo null custom_type_id falls back to Binary (aids debugging of new plugins vs backward compat) - Simplify map_err to forward original from_c_name() error messages - Add doc comments clarifying intentional field loss (bitstream_format, codec_private, profile, level) through C ABI pin-type declarations Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- sdks/plugin-sdk/native/src/conversions.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/sdks/plugin-sdk/native/src/conversions.rs b/sdks/plugin-sdk/native/src/conversions.rs index 41e0bf64..82dd33d0 100644 --- a/sdks/plugin-sdk/native/src/conversions.rs +++ b/sdks/plugin-sdk/native/src/conversions.rs @@ -74,8 +74,8 @@ pub fn packet_type_from_c(cpt_info: CPacketTypeInfo) -> Result Result Date: Tue, 7 Apr 2026 15:49:48 +0000 Subject: [PATCH 5/7] fix: address second round of review feedback on C ABI codec registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Make from_c_name strict (canonical lowercase only): remove 'avc'/'avc1' aliases from VideoCodec::from_c_name — serde aliases are for config deserialization, C ABI is a controlled interface with canonical names - Add tracing::warn! for EncodedAudio null custom_type_id fallback to Opus (matching the EncodedVideo pattern) - Make codec_name_to_cstring pub and replace 8 inline CString::new() .expect() sites in lib.rs macros — fully centralizes CString construction for codec names - Fix EncodedAudio doc comment: audio_codec -> custom_type_id - Replace alias roundtrip test with strict-mode rejection test Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- crates/core/src/types.rs | 15 ++++++---- sdks/plugin-sdk/native/src/conversions.rs | 6 +++- sdks/plugin-sdk/native/src/lib.rs | 36 +++++------------------ sdks/plugin-sdk/native/src/types.rs | 5 ++-- 4 files changed, 26 insertions(+), 36 deletions(-) diff --git a/crates/core/src/types.rs b/crates/core/src/types.rs index 685aef97..e0e48589 100644 --- a/crates/core/src/types.rs +++ b/crates/core/src/types.rs @@ -142,7 +142,7 @@ impl VideoCodec { pub fn from_c_name(name: &str) -> Result { match name { "vp9" => Ok(Self::Vp9), - "h264" | "avc" | "avc1" => Ok(Self::H264), + "h264" => Ok(Self::H264), "av1" => Ok(Self::Av1), other => Err(format!("unknown video codec name: {other:?}")), } @@ -971,10 +971,15 @@ mod tests { } #[test] - fn video_codec_from_c_name_aliases() { - // H264 should accept common aliases from container formats. - assert_eq!(VideoCodec::from_c_name("avc").unwrap(), VideoCodec::H264); - assert_eq!(VideoCodec::from_c_name("avc1").unwrap(), VideoCodec::H264); + fn codec_from_c_name_is_strict_canonical_only() { + // from_c_name only accepts canonical lowercase names from as_c_name(). + // Serde aliases ("avc", "avc1", "H264", etc.) are for config + // deserialization only — the C ABI is a controlled interface. + assert!(VideoCodec::from_c_name("avc").is_err()); + assert!(VideoCodec::from_c_name("avc1").is_err()); + assert!(VideoCodec::from_c_name("H264").is_err()); + assert!(AudioCodec::from_c_name("Opus").is_err()); + assert!(AudioCodec::from_c_name("AAC").is_err()); } #[test] diff --git a/sdks/plugin-sdk/native/src/conversions.rs b/sdks/plugin-sdk/native/src/conversions.rs index 82dd33d0..fd4886c9 100644 --- a/sdks/plugin-sdk/native/src/conversions.rs +++ b/sdks/plugin-sdk/native/src/conversions.rs @@ -93,6 +93,10 @@ pub fn packet_type_from_c(cpt_info: CPacketTypeInfo) -> Result RawVideoFormat { /// Codec names are compile-time ASCII constants that never contain interior /// null bytes, so `CString::new` cannot fail here. #[allow(clippy::expect_used)] // as_c_name() returns controlled constants; null bytes are a programmer error -fn codec_name_to_cstring(name: &str) -> CString { +pub fn codec_name_to_cstring(name: &str) -> CString { CString::new(name).expect("codec name from as_c_name() must not contain null bytes") } diff --git a/sdks/plugin-sdk/native/src/lib.rs b/sdks/plugin-sdk/native/src/lib.rs index 0b8cd1c3..4d062f5b 100644 --- a/sdks/plugin-sdk/native/src/lib.rs +++ b/sdks/plugin-sdk/native/src/lib.rs @@ -907,14 +907,10 @@ macro_rules! native_plugin_entry { )) } $crate::streamkit_core::types::PacketType::EncodedAudio(format) => { - Some(std::ffi::CString::new(format.codec.as_c_name()).expect( - "Codec name should not contain null bytes", - )) + Some($crate::conversions::codec_name_to_cstring(format.codec.as_c_name())) } $crate::streamkit_core::types::PacketType::EncodedVideo(format) => { - Some(std::ffi::CString::new(format.codec.as_c_name()).expect( - "Codec name should not contain null bytes", - )) + Some($crate::conversions::codec_name_to_cstring(format.codec.as_c_name())) } _ => None, }; @@ -1034,14 +1030,10 @@ macro_rules! native_plugin_entry { )) } $crate::streamkit_core::types::PacketType::EncodedAudio(format) => { - Some(std::ffi::CString::new(format.codec.as_c_name()).expect( - "Codec name should not contain null bytes", - )) + Some($crate::conversions::codec_name_to_cstring(format.codec.as_c_name())) } $crate::streamkit_core::types::PacketType::EncodedVideo(format) => { - Some(std::ffi::CString::new(format.codec.as_c_name()).expect( - "Codec name should not contain null bytes", - )) + Some($crate::conversions::codec_name_to_cstring(format.codec.as_c_name())) } _ => None, }; @@ -1439,16 +1431,10 @@ macro_rules! native_source_plugin_entry { ) }, $crate::streamkit_core::types::PacketType::EncodedAudio(format) => { - Some( - std::ffi::CString::new(format.codec.as_c_name()) - .expect("Codec name should not contain null bytes"), - ) + Some($crate::conversions::codec_name_to_cstring(format.codec.as_c_name())) }, $crate::streamkit_core::types::PacketType::EncodedVideo(format) => { - Some( - std::ffi::CString::new(format.codec.as_c_name()) - .expect("Codec name should not contain null bytes"), - ) + Some($crate::conversions::codec_name_to_cstring(format.codec.as_c_name())) }, _ => None, }; @@ -1566,16 +1552,10 @@ macro_rules! native_source_plugin_entry { .expect("Custom type_id should not contain null bytes"), ), $crate::streamkit_core::types::PacketType::EncodedAudio(format) => { - Some( - std::ffi::CString::new(format.codec.as_c_name()) - .expect("Codec name should not contain null bytes"), - ) + Some($crate::conversions::codec_name_to_cstring(format.codec.as_c_name())) }, $crate::streamkit_core::types::PacketType::EncodedVideo(format) => { - Some( - std::ffi::CString::new(format.codec.as_c_name()) - .expect("Codec name should not contain null bytes"), - ) + Some($crate::conversions::codec_name_to_cstring(format.codec.as_c_name())) }, _ => None, }; diff --git a/sdks/plugin-sdk/native/src/types.rs b/sdks/plugin-sdk/native/src/types.rs index 71f31856..f1303d5c 100644 --- a/sdks/plugin-sdk/native/src/types.rs +++ b/sdks/plugin-sdk/native/src/types.rs @@ -157,8 +157,9 @@ pub enum CPacketType { /// Binary packet that preserves optional `content_type` and `metadata` /// across the plugin ↔ host boundary. Points to a [`CBinaryPacket`]. BinaryWithMeta = 10, - /// Encoded audio with codec metadata. Uses `audio_codec` in - /// [`CPacketTypeInfo`] to identify the codec. + /// Encoded audio with codec metadata. Uses `custom_type_id` in + /// [`CPacketTypeInfo`] to carry the codec name (e.g. `"opus"`, `"aac"`). + /// Null `custom_type_id` defaults to Opus for backward compat. EncodedAudio = 11, } From 447ee3513e0b0c8abee985bb5ff83866a78baa5f Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Tue, 7 Apr 2026 15:49:59 +0000 Subject: [PATCH 6/7] style: apply rustfmt to macro codec_name_to_cstring calls Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- sdks/plugin-sdk/native/src/lib.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/sdks/plugin-sdk/native/src/lib.rs b/sdks/plugin-sdk/native/src/lib.rs index 4d062f5b..6dd1c853 100644 --- a/sdks/plugin-sdk/native/src/lib.rs +++ b/sdks/plugin-sdk/native/src/lib.rs @@ -1431,10 +1431,14 @@ macro_rules! native_source_plugin_entry { ) }, $crate::streamkit_core::types::PacketType::EncodedAudio(format) => { - Some($crate::conversions::codec_name_to_cstring(format.codec.as_c_name())) + Some($crate::conversions::codec_name_to_cstring( + format.codec.as_c_name(), + )) }, $crate::streamkit_core::types::PacketType::EncodedVideo(format) => { - Some($crate::conversions::codec_name_to_cstring(format.codec.as_c_name())) + Some($crate::conversions::codec_name_to_cstring( + format.codec.as_c_name(), + )) }, _ => None, }; @@ -1552,10 +1556,14 @@ macro_rules! native_source_plugin_entry { .expect("Custom type_id should not contain null bytes"), ), $crate::streamkit_core::types::PacketType::EncodedAudio(format) => { - Some($crate::conversions::codec_name_to_cstring(format.codec.as_c_name())) + Some($crate::conversions::codec_name_to_cstring( + format.codec.as_c_name(), + )) }, $crate::streamkit_core::types::PacketType::EncodedVideo(format) => { - Some($crate::conversions::codec_name_to_cstring(format.codec.as_c_name())) + Some($crate::conversions::codec_name_to_cstring( + format.codec.as_c_name(), + )) }, _ => None, }; From 9fd4085aca327c51e61a3e617946475a3aebd3b2 Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Tue, 7 Apr 2026 15:54:15 +0000 Subject: [PATCH 7/7] docs: add Panics section to codec_name_to_cstring doc comment Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- sdks/plugin-sdk/native/src/conversions.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/sdks/plugin-sdk/native/src/conversions.rs b/sdks/plugin-sdk/native/src/conversions.rs index fd4886c9..a8a4221d 100644 --- a/sdks/plugin-sdk/native/src/conversions.rs +++ b/sdks/plugin-sdk/native/src/conversions.rs @@ -194,6 +194,12 @@ pub const fn raw_video_format_from_c(cfmt: &CRawVideoFormat) -> RawVideoFormat { /// /// Codec names are compile-time ASCII constants that never contain interior /// null bytes, so `CString::new` cannot fail here. +/// +/// # Panics +/// +/// Panics if `name` contains an interior null byte. This is a programmer +/// error — `as_c_name()` values are controlled constants that never contain +/// null bytes. #[allow(clippy::expect_used)] // as_c_name() returns controlled constants; null bytes are a programmer error pub fn codec_name_to_cstring(name: &str) -> CString { CString::new(name).expect("codec name from as_c_name() must not contain null bytes")