Skip to content

Commit 87e3828

Browse files
staging-devin-ai-integration[bot]streamkit-devinstreamer45
authored
refactor: formalize C ABI codec registry via as_c_name/from_c_name (#265)
* refactor: formalize C ABI codec registry via as_c_name/from_c_name 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 <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com> * 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 <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com> * 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 <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com> * 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 <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com> * fix: address second round of review feedback on C ABI codec registry - 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 <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com> * style: apply rustfmt to macro codec_name_to_cstring calls Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com> * docs: add Panics section to codec_name_to_cstring doc comment Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com> --------- Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-authored-by: StreamKit Devin <devin@streamkit.dev> Co-authored-by: Claudio Costa <cstcld91@gmail.com>
1 parent f28ab86 commit 87e3828

File tree

4 files changed

+292
-73
lines changed

4 files changed

+292
-73
lines changed

crates/core/src/types.rs

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,29 @@ impl AudioCodec {
8181
Self::Aac => 21_333,
8282
}
8383
}
84+
85+
/// Canonical lowercase name used in the C ABI (`custom_type_id`).
86+
///
87+
/// Adding a new `AudioCodec` variant? Add its name here and in
88+
/// [`Self::from_c_name`] — that's the **only** place codec-name
89+
/// strings need to live.
90+
pub const fn as_c_name(self) -> &'static str {
91+
match self {
92+
Self::Opus => "opus",
93+
Self::Aac => "aac",
94+
}
95+
}
96+
97+
/// Parse a C ABI codec name back to an `AudioCodec`.
98+
///
99+
/// Accepts the canonical lowercase names produced by [`Self::as_c_name`].
100+
pub fn from_c_name(name: &str) -> Result<Self, String> {
101+
match name {
102+
"opus" => Ok(Self::Opus),
103+
"aac" => Ok(Self::Aac),
104+
other => Err(format!("unknown audio codec name: {other:?}")),
105+
}
106+
}
84107
}
85108

86109
/// Supported encoded video codecs.
@@ -99,6 +122,33 @@ pub enum VideoCodec {
99122
Av1,
100123
}
101124

125+
impl VideoCodec {
126+
/// Canonical lowercase name used in the C ABI (`custom_type_id`).
127+
///
128+
/// Adding a new `VideoCodec` variant? Add its name here and in
129+
/// [`Self::from_c_name`] — that's the **only** place codec-name
130+
/// strings need to live.
131+
pub const fn as_c_name(self) -> &'static str {
132+
match self {
133+
Self::Vp9 => "vp9",
134+
Self::H264 => "h264",
135+
Self::Av1 => "av1",
136+
}
137+
}
138+
139+
/// Parse a C ABI codec name back to a `VideoCodec`.
140+
///
141+
/// Accepts the canonical lowercase names produced by [`Self::as_c_name`].
142+
pub fn from_c_name(name: &str) -> Result<Self, String> {
143+
match name {
144+
"vp9" => Ok(Self::Vp9),
145+
"h264" => Ok(Self::H264),
146+
"av1" => Ok(Self::Av1),
147+
other => Err(format!("unknown video codec name: {other:?}")),
148+
}
149+
}
150+
}
151+
102152
/// Bitstream format hints for video codecs (primarily H264).
103153
#[derive(Debug, PartialEq, Eq, Clone, Copy, Serialize, Deserialize, JsonSchema, TS)]
104154
#[ts(export)]
@@ -897,4 +947,44 @@ mod tests {
897947

898948
assert_eq!(pool.stats().buckets[0].available, 1);
899949
}
950+
951+
// ── Codec C-name roundtrip tests ───────────────────────────────────
952+
953+
#[test]
954+
fn audio_codec_c_name_roundtrip() {
955+
for codec in [AudioCodec::Opus, AudioCodec::Aac] {
956+
let name = codec.as_c_name();
957+
let parsed = AudioCodec::from_c_name(name)
958+
.unwrap_or_else(|e| panic!("roundtrip failed for {codec:?}: {e}"));
959+
assert_eq!(codec, parsed, "roundtrip mismatch for {name:?}");
960+
}
961+
}
962+
963+
#[test]
964+
fn video_codec_c_name_roundtrip() {
965+
for codec in [VideoCodec::Vp9, VideoCodec::H264, VideoCodec::Av1] {
966+
let name = codec.as_c_name();
967+
let parsed = VideoCodec::from_c_name(name)
968+
.unwrap_or_else(|e| panic!("roundtrip failed for {codec:?}: {e}"));
969+
assert_eq!(codec, parsed, "roundtrip mismatch for {name:?}");
970+
}
971+
}
972+
973+
#[test]
974+
fn codec_from_c_name_is_strict_canonical_only() {
975+
// from_c_name only accepts canonical lowercase names from as_c_name().
976+
// Serde aliases ("avc", "avc1", "H264", etc.) are for config
977+
// deserialization only — the C ABI is a controlled interface.
978+
assert!(VideoCodec::from_c_name("avc").is_err());
979+
assert!(VideoCodec::from_c_name("avc1").is_err());
980+
assert!(VideoCodec::from_c_name("H264").is_err());
981+
assert!(AudioCodec::from_c_name("Opus").is_err());
982+
assert!(AudioCodec::from_c_name("AAC").is_err());
983+
}
984+
985+
#[test]
986+
fn codec_from_c_name_unknown_errors() {
987+
assert!(AudioCodec::from_c_name("mp3").is_err());
988+
assert!(VideoCodec::from_c_name("hevc").is_err());
989+
}
900990
}

sdks/plugin-sdk/native/src/conversions.rs

Lines changed: 154 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,8 @@ use std::sync::Arc;
1818
use streamkit_core::frame_pool::{PooledSamples, PooledVideoData};
1919
use streamkit_core::types::{
2020
AudioCodec, AudioFormat, AudioFrame, CustomEncoding, CustomPacketData, EncodedAudioFormat,
21-
Packet, PacketMetadata, PacketType, PixelFormat, RawVideoFormat, SampleFormat,
22-
TranscriptionData, VideoFrame,
21+
EncodedVideoFormat, Packet, PacketMetadata, PacketType, PixelFormat, RawVideoFormat,
22+
SampleFormat, TranscriptionData, VideoCodec, VideoFrame,
2323
};
2424

2525
/// Convert C packet type info to Rust PacketType
@@ -62,27 +62,48 @@ pub fn packet_type_from_c(cpt_info: CPacketTypeInfo) -> Result<PacketType, Strin
6262
Ok(PacketType::RawVideo(raw_video_format_from_c(c_fmt)))
6363
},
6464
CPacketType::EncodedVideo => {
65-
// TODO: Add CVideoCodec enum to carry codec through the C ABI.
66-
// Until then, EncodedVideo is accepted as a discriminant for pin
67-
// declarations but the codec field is meaningless. Using Binary
68-
// as the packet-level representation (see `packet_from_c`) avoids
69-
// silently mislabelling the codec.
70-
Ok(PacketType::Binary)
65+
// The codec name is carried in `custom_type_id` (same pattern as
66+
// EncodedAudio). Null `custom_type_id` falls back to Binary for
67+
// backward compat with plugins compiled before codec strings were
68+
// added to EncodedVideo.
69+
if cpt_info.custom_type_id.is_null() {
70+
tracing::warn!(
71+
"EncodedVideo pin has null custom_type_id; \
72+
falling back to Binary (pre-codec-string plugin?)"
73+
);
74+
Ok(PacketType::Binary)
75+
} else {
76+
let name = unsafe { c_str_to_string(cpt_info.custom_type_id) }?;
77+
let codec =
78+
VideoCodec::from_c_name(&name).map_err(|e| format!("EncodedVideo: {e}"))?;
79+
// Note: bitstream_format, codec_private, profile, and level
80+
// are not carried through the C ABI — this conversion is used
81+
// for pin-type declarations only, not runtime packet data.
82+
Ok(PacketType::EncodedVideo(EncodedVideoFormat {
83+
codec,
84+
bitstream_format: None,
85+
codec_private: None,
86+
profile: None,
87+
level: None,
88+
}))
89+
}
7190
},
7291
CPacketType::EncodedAudio => {
7392
// The codec name is carried in `custom_type_id` to avoid changing
7493
// the CPacketTypeInfo struct layout (see ABI stability note).
7594
let codec = if cpt_info.custom_type_id.is_null() {
7695
// Default to Opus when no codec name is provided.
96+
tracing::warn!(
97+
"EncodedAudio pin has null custom_type_id; \
98+
falling back to Opus (pre-codec-string plugin?)"
99+
);
77100
AudioCodec::Opus
78101
} else {
79102
let name = unsafe { c_str_to_string(cpt_info.custom_type_id) }?;
80-
match name.as_str() {
81-
"opus" => AudioCodec::Opus,
82-
"aac" => AudioCodec::Aac,
83-
other => return Err(format!("Unknown EncodedAudio codec name: {other:?}")),
84-
}
103+
AudioCodec::from_c_name(&name).map_err(|e| format!("EncodedAudio: {e}"))?
85104
};
105+
// Note: codec_private is not carried through the C ABI — this
106+
// conversion is used for pin-type declarations only.
86107
Ok(PacketType::EncodedAudio(EncodedAudioFormat { codec, codec_private: None }))
87108
},
88109
CPacketType::Binary | CPacketType::BinaryWithMeta => Ok(PacketType::Binary),
@@ -169,6 +190,21 @@ pub const fn raw_video_format_from_c(cfmt: &CRawVideoFormat) -> RawVideoFormat {
169190
}
170191
}
171192

193+
/// Build a `CString` from a codec name returned by `as_c_name()`.
194+
///
195+
/// Codec names are compile-time ASCII constants that never contain interior
196+
/// null bytes, so `CString::new` cannot fail here.
197+
///
198+
/// # Panics
199+
///
200+
/// Panics if `name` contains an interior null byte. This is a programmer
201+
/// error — `as_c_name()` values are controlled constants that never contain
202+
/// null bytes.
203+
#[allow(clippy::expect_used)] // as_c_name() returns controlled constants; null bytes are a programmer error
204+
pub fn codec_name_to_cstring(name: &str) -> CString {
205+
CString::new(name).expect("codec name from as_c_name() must not contain null bytes")
206+
}
207+
172208
/// Ancillary data kept alive alongside a `CPacketTypeInfo`.
173209
///
174210
/// `packet_type_to_c` returns this alongside the info struct so that
@@ -179,6 +215,10 @@ pub enum CPacketTypeOwned {
179215
None,
180216
Audio(CAudioFormat),
181217
Video(CRawVideoFormat),
218+
/// Null-terminated codec name derived from `AudioCodec::as_c_name()` or
219+
/// `VideoCodec::as_c_name()`. The `custom_type_id` pointer in the
220+
/// accompanying `CPacketTypeInfo` points into this `CString`.
221+
CodecName(CString),
182222
}
183223

184224
/// Convert Rust PacketType to C representation.
@@ -224,21 +264,18 @@ pub fn packet_type_to_c(pt: &PacketType) -> (CPacketTypeInfo, CPacketTypeOwned)
224264
CPacketTypeOwned::None,
225265
)
226266
} else {
227-
// Carry the codec name in `custom_type_id` (reusing the
228-
// existing pointer field to avoid changing the struct layout).
229-
let codec_name: &'static [u8] = match format.codec {
230-
AudioCodec::Opus => b"opus\0",
231-
AudioCodec::Aac => b"aac\0",
232-
_ => b"unknown\0",
233-
};
267+
// Derive the null-terminated codec name from as_c_name() so
268+
// the canonical name lives in exactly one place.
269+
let name = codec_name_to_cstring(format.codec.as_c_name());
270+
let ptr = name.as_ptr();
234271
(
235272
CPacketTypeInfo {
236273
type_discriminant: CPacketType::EncodedAudio,
237274
audio_format: std::ptr::null(),
238-
custom_type_id: codec_name.as_ptr().cast::<c_char>(),
275+
custom_type_id: ptr,
239276
raw_video_format: std::ptr::null(),
240277
},
241-
CPacketTypeOwned::None,
278+
CPacketTypeOwned::CodecName(name),
242279
)
243280
}
244281
},
@@ -282,15 +319,21 @@ pub fn packet_type_to_c(pt: &PacketType) -> (CPacketTypeInfo, CPacketTypeOwned)
282319
CPacketTypeOwned::Video(c_fmt),
283320
)
284321
},
285-
PacketType::EncodedVideo(_) => (
286-
CPacketTypeInfo {
287-
type_discriminant: CPacketType::EncodedVideo,
288-
audio_format: std::ptr::null(),
289-
custom_type_id: std::ptr::null(),
290-
raw_video_format: std::ptr::null(),
291-
},
292-
CPacketTypeOwned::None,
293-
),
322+
PacketType::EncodedVideo(format) => {
323+
// Derive the null-terminated codec name from as_c_name() so
324+
// the canonical name lives in exactly one place.
325+
let name = codec_name_to_cstring(format.codec.as_c_name());
326+
let ptr = name.as_ptr();
327+
(
328+
CPacketTypeInfo {
329+
type_discriminant: CPacketType::EncodedVideo,
330+
audio_format: std::ptr::null(),
331+
custom_type_id: ptr,
332+
raw_video_format: std::ptr::null(),
333+
},
334+
CPacketTypeOwned::CodecName(name),
335+
)
336+
},
294337
PacketType::Binary => (
295338
CPacketTypeInfo {
296339
type_discriminant: CPacketType::Binary,
@@ -990,4 +1033,84 @@ mod tests {
9901033
assert_eq!(repr.packet.packet_type, CPacketType::Binary, "should remain Binary");
9911034
assert_eq!(repr.packet.len, payload.len());
9921035
}
1036+
1037+
// ── EncodedVideo codec roundtrip tests ─────────────────────────────
1038+
1039+
/// `packet_type_to_c` → `packet_type_from_c` must roundtrip all video
1040+
/// codecs through the `custom_type_id` string pointer.
1041+
#[test]
1042+
fn encoded_video_codec_roundtrip_via_c() {
1043+
use streamkit_core::types::{EncodedVideoFormat, VideoCodec};
1044+
1045+
for codec in [VideoCodec::Vp9, VideoCodec::H264, VideoCodec::Av1] {
1046+
let pt = PacketType::EncodedVideo(EncodedVideoFormat {
1047+
codec,
1048+
bitstream_format: None,
1049+
codec_private: None,
1050+
profile: None,
1051+
level: None,
1052+
});
1053+
1054+
let (info, _owned) = packet_type_to_c(&pt);
1055+
assert_eq!(
1056+
info.type_discriminant,
1057+
CPacketType::EncodedVideo,
1058+
"discriminant mismatch for {codec:?}"
1059+
);
1060+
assert!(!info.custom_type_id.is_null(), "custom_type_id should be set for {codec:?}");
1061+
1062+
let roundtripped = packet_type_from_c(info)
1063+
.unwrap_or_else(|e| panic!("roundtrip failed for {codec:?}: {e}"));
1064+
1065+
match roundtripped {
1066+
PacketType::EncodedVideo(fmt) => {
1067+
assert_eq!(fmt.codec, codec, "codec mismatch after roundtrip");
1068+
},
1069+
other => panic!("expected EncodedVideo, got {other:?}"),
1070+
}
1071+
}
1072+
}
1073+
1074+
/// `EncodedVideo` with null `custom_type_id` falls back to `Binary`
1075+
/// (backward compat with pre-codec-string plugins).
1076+
#[test]
1077+
fn encoded_video_null_codec_falls_back_to_binary() {
1078+
let info = CPacketTypeInfo {
1079+
type_discriminant: CPacketType::EncodedVideo,
1080+
audio_format: std::ptr::null(),
1081+
custom_type_id: std::ptr::null(),
1082+
raw_video_format: std::ptr::null(),
1083+
};
1084+
let pt = packet_type_from_c(info)
1085+
.unwrap_or_else(|e| panic!("null custom_type_id should fall back to Binary: {e}"));
1086+
assert_eq!(pt, PacketType::Binary);
1087+
}
1088+
1089+
/// `EncodedAudio` roundtrips correctly through `custom_type_id`.
1090+
#[test]
1091+
fn encoded_audio_codec_roundtrip_via_c() {
1092+
for codec in [AudioCodec::Opus, AudioCodec::Aac] {
1093+
let pt = PacketType::EncodedAudio(EncodedAudioFormat { codec, codec_private: None });
1094+
1095+
let (info, _owned) = packet_type_to_c(&pt);
1096+
1097+
// Opus without codec_private uses the legacy OpusAudio discriminant.
1098+
if codec == AudioCodec::Opus {
1099+
assert_eq!(info.type_discriminant, CPacketType::OpusAudio);
1100+
} else {
1101+
assert_eq!(info.type_discriminant, CPacketType::EncodedAudio);
1102+
assert!(!info.custom_type_id.is_null());
1103+
}
1104+
1105+
let roundtripped = packet_type_from_c(info)
1106+
.unwrap_or_else(|e| panic!("roundtrip failed for {codec:?}: {e}"));
1107+
1108+
match roundtripped {
1109+
PacketType::EncodedAudio(fmt) => {
1110+
assert_eq!(fmt.codec, codec, "codec mismatch after roundtrip");
1111+
},
1112+
other => panic!("expected EncodedAudio, got {other:?}"),
1113+
}
1114+
}
1115+
}
9931116
}

0 commit comments

Comments
 (0)