From 6bed427147e6ce338d69db8a14d9854430c4df59 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 21 Jul 2026 13:26:30 -0700 Subject: [PATCH 01/11] refactor(token)!: encapsulate Key so cached crypto material can't go stale `Key` exposed `pub` fields while caching the derived `EncodingKey` and `DecodingKey` in a `OnceLock`. Signing or verifying once populated that cache; mutating `algorithm` or `key` afterwards left it in place, so the next call wrote the new `alg` into the JWT header while signing with the old key material. `scope` was documented as an immutable authorization ceiling but was a `pub` field anyone could overwrite, bypassing the validation in `with_scope`. Split the serialized JWK representation (`Jwk`, all `pub` fields) from an immutable runtime `Key` with private fields, accessors, and validating constructors (`TryFrom`, `from_str`, `generate`, consuming `with_scope` / `with_operations`). Nothing can be reassigned after construction, so the cache can never disagree with the metadata it was derived from. Also fixes a latent serde bug found on the way: a bare `Jwk::deserialize` resolves to serde's `remote = "Self"` inherent method rather than the trait impl, silently skipping the `kty` -> `oct` backfill that keeps previously issued tokens valid. No wire, JWK, or JWT change. `js/token` needs no matching change: it re-imports the key on every sign and verify, so it has no cached-material equivalent. Co-Authored-By: Claude Opus 4.8 --- rs/moq-token/src/generate.rs | 9 +- rs/moq-token/src/key.rs | 407 ++++++++++++++++++++++++----------- rs/moq-token/src/set.rs | 38 ++-- 3 files changed, 303 insertions(+), 151 deletions(-) diff --git a/rs/moq-token/src/generate.rs b/rs/moq-token/src/generate.rs index 8c7b692949..a7d18aa51a 100644 --- a/rs/moq-token/src/generate.rs +++ b/rs/moq-token/src/generate.rs @@ -1,5 +1,5 @@ use crate::error::KeyError; -use crate::{Algorithm, EllipticCurve, Key, KeyOperation, KeyType, RsaPublicKey}; +use crate::{Algorithm, EllipticCurve, Jwk, Key, KeyOperation, KeyType, RsaPublicKey}; use aws_lc_rs::encoding::AsBigEndian; use aws_lc_rs::signature::KeyPair; use p256::elliptic_curve::array::typenum::Unsigned; @@ -21,15 +21,14 @@ pub fn generate(algorithm: Algorithm, id: Option) -> crate::Result Algorithm::EdDSA => generate_ed25519_key(), }; - Ok(Key { + Jwk { kid: id, operations: [KeyOperation::Sign, KeyOperation::Verify].into(), algorithm, key: key?, scope: None, - decode: Default::default(), - encode: Default::default(), - }) + } + .try_into() } fn generate_hmac_key() -> crate::Result { diff --git a/rs/moq-token/src/key.rs b/rs/moq-token/src/key.rs index 8fcae74668..2d77370ab0 100644 --- a/rs/moq-token/src/key.rs +++ b/rs/moq-token/src/key.rs @@ -141,9 +141,12 @@ pub struct RsaAdditionalPrime { /// JWK, almost to spec () but not quite the same /// because it's annoying to implement. +/// +/// This is the serialized form of a key, with plain fields you can build and edit. It is not +/// usable on its own: convert it into a [`Key`] via `Key::try_from` to sign or verify anything. #[derive(Clone, Serialize, Deserialize)] #[serde(remote = "Self")] -pub struct Key { +pub struct Jwk { /// The algorithm used by the key. #[serde(rename = "alg")] pub algorithm: Algorithm, @@ -152,7 +155,7 @@ pub struct Key { #[serde(rename = "key_ops")] pub operations: HashSet, - /// Defaults to KeyType::OCT + /// The key material. Defaults to [`KeyType::OCT`] when `kty` is absent. #[serde(flatten)] pub key: KeyType, @@ -160,19 +163,12 @@ pub struct Key { #[serde(skip_serializing_if = "Option::is_none")] pub kid: Option, - /// Optional immutable authorization limits for tokens signed by this key. + /// Optional authorization limits for tokens signed by this key. #[serde(default, skip_serializing_if = "Option::is_none")] pub scope: Option, - - // Cached for performance reasons, unfortunately. - #[serde(skip)] - pub(crate) decode: OnceLock, - - #[serde(skip)] - pub(crate) encode: OnceLock, } -impl<'de> Deserialize<'de> for Key { +impl<'de> Deserialize<'de> for Jwk { fn deserialize(deserializer: D) -> Result where D: Deserializer<'de>, @@ -192,7 +188,7 @@ impl<'de> Deserialize<'de> for Key { } } -impl Serialize for Key { +impl Serialize for Jwk { fn serialize(&self, serializer: S) -> Result where S: Serializer, @@ -201,6 +197,78 @@ impl Serialize for Key { } } +/// A validated key, ready to sign and verify tokens. +/// +/// The fields are fixed at construction: derived crypto material is cached on first use, so a key +/// that could be mutated would sign with stale material. Build one from a [`Jwk`], from +/// [`Key::generate`], or by parsing with [`Key::from_str`], then use the builders to derive a new +/// key rather than editing an existing one. +#[derive(Clone)] +pub struct Key { + algorithm: Algorithm, + operations: HashSet, + key: KeyType, + kid: Option, + scope: Option, + + // Cached for performance reasons, unfortunately. + decode: OnceLock, + encode: OnceLock, +} + +impl TryFrom for Key { + type Error = crate::Error; + + fn try_from(jwk: Jwk) -> crate::Result { + if let Some(scope) = &jwk.scope { + scope.validate()?; + } + + Ok(Self { + algorithm: jwk.algorithm, + operations: jwk.operations, + key: jwk.key, + kid: jwk.kid, + scope: jwk.scope, + decode: Default::default(), + encode: Default::default(), + }) + } +} + +impl From<&Key> for Jwk { + fn from(key: &Key) -> Self { + Self { + algorithm: key.algorithm, + operations: key.operations.clone(), + key: key.key.clone(), + kid: key.kid.clone(), + scope: key.scope.clone(), + } + } +} + +impl<'de> Deserialize<'de> for Key { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + // Call the trait impl explicitly: the bare path would resolve to the inherent method that + // serde's `remote = "Self"` generates, skipping the `kty` default above. + let jwk = ::deserialize(deserializer)?; + Key::try_from(jwk).map_err(serde::de::Error::custom) + } +} + +impl Serialize for Key { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + Serialize::serialize(&Jwk::from(self), serializer) + } +} + impl fmt::Debug for Key { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("Key") @@ -213,6 +281,31 @@ impl fmt::Debug for Key { } impl Key { + /// The algorithm this key signs and verifies with. + pub fn algorithm(&self) -> Algorithm { + self.algorithm + } + + /// The operations this key is allowed to perform. + pub fn operations(&self) -> &HashSet { + &self.operations + } + + /// The key material, including its JWK type. + pub fn key_type(&self) -> &KeyType { + &self.key + } + + /// The key ID (`kid`), used to select this key out of a [`crate::KeySet`]. + pub fn kid(&self) -> Option<&crate::KeyId> { + self.kid.as_ref() + } + + /// The authorization ceiling on tokens this key signs or verifies, if any. + pub fn scope(&self) -> Option<&crate::Scope> { + self.scope.as_ref() + } + /// Parse a key from a string, auto-detecting JSON or base64url encoding. #[allow(clippy::should_implement_trait)] pub fn from_str(s: &str) -> crate::Result { @@ -252,6 +345,10 @@ impl Key { Ok(()) } + /// Derive a verify-only copy of this key, dropping the private material. + /// + /// Fails for symmetric (`oct`) keys, which have no public half, and for a key that cannot + /// verify in the first place. pub fn to_public(&self) -> crate::Result { if !self.operations.contains(&KeyOperation::Verify) { return Err(KeyError::VerifyUnsupported.into()); @@ -483,13 +580,22 @@ impl Key { generate(algorithm, id) } - /// Attach an immutable authorization scope to this key. + /// Derive a key with an authorization scope attached, capping what its tokens may grant. + /// + /// The scope is validated here, and it is the only way to set one, so a key can never carry a + /// scope that permits nothing. pub fn with_scope(mut self, scope: crate::Scope) -> crate::Result { scope.validate()?; self.scope = Some(scope); Ok(self) } + /// Derive a key restricted to the given operations. + pub fn with_operations(mut self, operations: impl IntoIterator) -> Self { + self.operations = operations.into_iter().collect(); + self + } + fn validate_scope(&self, claims: &Claims) -> crate::Result<()> { if let Some(scope) = &self.scope { scope.validate()?; @@ -560,7 +666,7 @@ mod tests { use std::time::{Duration, SystemTime}; fn create_test_key() -> Key { - Key { + Jwk { algorithm: Algorithm::HS256, operations: [KeyOperation::Sign, KeyOperation::Verify].into(), key: KeyType::OCT { @@ -568,9 +674,9 @@ mod tests { }, kid: Some(crate::KeyId::decode("test-key-1").unwrap()), scope: None, - decode: Default::default(), - encode: Default::default(), } + .try_into() + .unwrap() } fn create_test_claims() -> Claims { @@ -589,15 +695,15 @@ mod tests { let json = key.to_str().unwrap(); let loaded_key = Key::from_str(&json).unwrap(); - assert_eq!(loaded_key.algorithm, key.algorithm); - assert_eq!(loaded_key.operations, key.operations); - match (loaded_key.key, key.key) { + assert_eq!(loaded_key.algorithm(), key.algorithm()); + assert_eq!(loaded_key.operations(), key.operations()); + match (loaded_key.key_type(), key.key_type()) { (KeyType::OCT { secret: loaded_secret }, KeyType::OCT { secret }) => { assert_eq!(loaded_secret, secret); } _ => panic!("Expected OCT key"), } - assert_eq!(loaded_key.kid, key.kid); + assert_eq!(loaded_key.kid(), key.kid()); } /// Tests whether Key::from_str() works for keys without a kty value to fall back to OCT @@ -609,7 +715,7 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - if let KeyType::OCT { ref secret, .. } = key.key { + if let KeyType::OCT { secret, .. } = key.key_type() { let base64_key = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(secret); assert_eq!(base64_key, "Fp8kipWUJeUFqeSqWym_tRC_tyI8z-QpqopIGrbrD68"); } else { @@ -620,10 +726,10 @@ mod tests { // Round-trip through from_str and verify fields let loaded = Key::from_str(&key_str).unwrap(); - assert_eq!(loaded.algorithm, Algorithm::HS256); - assert!(loaded.operations.contains(&KeyOperation::Sign)); - assert!(loaded.operations.contains(&KeyOperation::Verify)); - assert!(matches!(loaded.key, KeyType::OCT { .. })); + assert_eq!(loaded.algorithm(), Algorithm::HS256); + assert!(loaded.operations().contains(&KeyOperation::Sign)); + assert!(loaded.operations().contains(&KeyOperation::Verify)); + assert!(matches!(loaded.key_type(), KeyType::OCT { .. })); } #[test] @@ -642,10 +748,10 @@ mod tests { // Round-trip through from_str let loaded = Key::from_str(&encoded).unwrap(); - assert_eq!(loaded.algorithm, Algorithm::HS256); - assert_eq!(loaded.kid, key.kid); - assert!(loaded.operations.contains(&KeyOperation::Sign)); - assert!(loaded.operations.contains(&KeyOperation::Verify)); + assert_eq!(loaded.algorithm(), Algorithm::HS256); + assert_eq!(loaded.kid(), key.kid()); + assert!(loaded.operations().contains(&KeyOperation::Sign)); + assert!(loaded.operations().contains(&KeyOperation::Verify)); } #[test] @@ -687,10 +793,63 @@ mod tests { assert!(matches!(scoped.verify(&forged), Err(crate::Error::ScopeExceeded))); } + /// A key's crypto material is derived once and cached, so the fields it was derived from must + /// stay fixed. Changing the algorithm means building a new key, which derives fresh material. + #[test] + fn test_key_derived_material_never_stale() { + let claims = Claims { + root: "test-path".into(), + publish: vec!["test-pub".into()], + ..Default::default() + }; + + // Sign once so the encode/decode caches are populated. + let key = create_test_key(); + let token = key.sign(&claims).unwrap(); + assert!(key.encode.get().is_some()); + + // The only way to change the algorithm is to build another key, which starts with an empty + // cache and therefore signs with material matching the header it writes. + let mut jwk = Jwk::from(&key); + jwk.algorithm = Algorithm::HS384; + let derived = Key::try_from(jwk).unwrap(); + assert!(derived.encode.get().is_none()); + + let derived_token = derived.sign(&claims).unwrap(); + assert_ne!(token, derived_token); + + // The derived key agrees with one parsed cold from the same JWK, and the original key + // rejects the token it did not sign. + let cold = Key::from_str(&derived.to_str().unwrap()).unwrap(); + assert_eq!(derived_token, cold.sign(&claims).unwrap()); + assert!(cold.verify(&derived_token).is_ok()); + assert!(key.verify(&derived_token).is_err()); + } + + /// A scope can only be attached through the validating builder, and the serde path validates + /// too, so a key can never carry a scope that grants nothing. + #[test] + fn test_key_scope_requires_validation() { + let key = create_test_key(); + assert!(key.scope().is_none()); + + let useless = crate::Scope::default(); + assert!(matches!( + key.clone().with_scope(useless.clone()), + Err(crate::Error::UselessScope) + )); + + let mut jwk = Jwk::from(&key); + jwk.scope = Some(useless); + assert!(matches!(Key::try_from(jwk), Err(crate::Error::UselessScope))); + + let json = r#"{"alg":"HS256","key_ops":["sign"],"k":"Fp8kipWUJeUFqeSqWym_tRC_tyI8z-QpqopIGrbrD68","scope":{}}"#; + assert!(Key::from_str(json).is_err()); + } + #[test] fn test_key_sign_no_permission() { - let mut key = create_test_key(); - key.operations = [KeyOperation::Verify].into(); + let key = create_test_key().with_operations([KeyOperation::Verify]); let claims = create_test_claims(); let result = key.sign(&claims); @@ -733,8 +892,7 @@ mod tests { #[test] fn test_key_verify_no_permission() { - let mut key = create_test_key(); - key.operations = [KeyOperation::Sign].into(); + let key = create_test_key().with_operations([KeyOperation::Sign]); let result = key.verify("some.jwt.token"); assert!(result.is_err()); @@ -819,12 +977,12 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert_eq!(key.algorithm, Algorithm::HS256); - assert_eq!(key.kid, Some(crate::KeyId::decode("test-id").unwrap())); - assert_eq!(key.operations, [KeyOperation::Sign, KeyOperation::Verify].into()); + assert_eq!(key.algorithm(), Algorithm::HS256); + assert_eq!(key.kid(), Some(&crate::KeyId::decode("test-id").unwrap())); + assert_eq!(key.operations(), &[KeyOperation::Sign, KeyOperation::Verify].into()); - match key.key { - KeyType::OCT { ref secret } => assert_eq!(secret.len(), 32), + match key.key_type() { + KeyType::OCT { secret } => assert_eq!(secret.len(), 32), _ => panic!("Expected OCT key"), } } @@ -835,10 +993,10 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert_eq!(key.algorithm, Algorithm::HS384); + assert_eq!(key.algorithm(), Algorithm::HS384); - match key.key { - KeyType::OCT { ref secret } => assert_eq!(secret.len(), 48), + match key.key_type() { + KeyType::OCT { secret } => assert_eq!(secret.len(), 48), _ => panic!("Expected OCT key"), } } @@ -849,10 +1007,10 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert_eq!(key.algorithm, Algorithm::HS512); + assert_eq!(key.algorithm(), Algorithm::HS512); - match key.key { - KeyType::OCT { ref secret } => assert_eq!(secret.len(), 64), + match key.key_type() { + KeyType::OCT { secret } => assert_eq!(secret.len(), 64), _ => panic!("Expected OCT key"), } } @@ -863,13 +1021,10 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert_eq!(key.algorithm, Algorithm::RS512); - assert!(matches!(key.key, KeyType::RSA { .. })); - match key.key { - KeyType::RSA { - ref public, - ref private, - } => { + assert_eq!(key.algorithm(), Algorithm::RS512); + assert!(matches!(key.key_type(), KeyType::RSA { .. })); + match key.key_type() { + KeyType::RSA { public, private } => { assert!(private.is_some()); assert_eq!(public.n.len(), 256); assert_eq!(public.e.len(), 3); @@ -884,8 +1039,8 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert_eq!(key.algorithm, Algorithm::ES256); - assert!(matches!(key.key, KeyType::EC { .. })) + assert_eq!(key.algorithm(), Algorithm::ES256); + assert!(matches!(key.key_type(), KeyType::EC { .. })) } #[test] @@ -894,8 +1049,8 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert_eq!(key.algorithm, Algorithm::PS512); - assert!(matches!(key.key, KeyType::RSA { .. })); + assert_eq!(key.algorithm(), Algorithm::PS512); + assert!(matches!(key.key_type(), KeyType::RSA { .. })); } #[test] @@ -904,8 +1059,8 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert_eq!(key.algorithm, Algorithm::EdDSA); - assert!(matches!(key.key, KeyType::OKP { .. })); + assert_eq!(key.algorithm(), Algorithm::EdDSA); + assert!(matches!(key.key_type(), KeyType::OKP { .. })); } #[test] @@ -914,9 +1069,9 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert_eq!(key.algorithm, Algorithm::HS256); - assert_eq!(key.kid, None); - assert_eq!(key.operations, [KeyOperation::Sign, KeyOperation::Verify].into()); + assert_eq!(key.algorithm(), Algorithm::HS256); + assert_eq!(key.kid(), None); + assert_eq!(key.operations(), &[KeyOperation::Sign, KeyOperation::Verify].into()); } #[test] @@ -934,16 +1089,16 @@ mod tests { let key = key.unwrap(); let public_key = key.to_public().unwrap(); - assert_eq!(key.kid, public_key.kid); - assert_eq!(public_key.operations, [KeyOperation::Verify].into()); + assert_eq!(key.kid(), public_key.kid()); + assert_eq!(public_key.operations(), &[KeyOperation::Verify].into()); assert!(public_key.encode.get().is_none()); assert!(public_key.decode.get().is_none()); - assert!(matches!(public_key.key, KeyType::RSA { .. })); + assert!(matches!(public_key.key_type(), KeyType::RSA { .. })); - if let KeyType::RSA { public, private } = &public_key.key { + if let KeyType::RSA { public, private } = public_key.key_type() { assert!(private.is_none()); - if let KeyType::RSA { public: src_public, .. } = &key.key { + if let KeyType::RSA { public: src_public, .. } = key.key_type() { assert_eq!(public.e, src_public.e); assert_eq!(public.n, src_public.n); } else { @@ -961,13 +1116,13 @@ mod tests { let key = key.unwrap(); let public_key = key.to_public().unwrap(); - assert_eq!(key.kid, public_key.kid); - assert_eq!(public_key.operations, [KeyOperation::Verify].into()); + assert_eq!(key.kid(), public_key.kid()); + assert_eq!(public_key.operations(), &[KeyOperation::Verify].into()); assert!(public_key.encode.get().is_none()); assert!(public_key.decode.get().is_none()); - assert!(matches!(public_key.key, KeyType::EC { .. })); + assert!(matches!(public_key.key_type(), KeyType::EC { .. })); - if let KeyType::EC { x, y, d, curve } = &public_key.key { + if let KeyType::EC { x, y, d, curve } = public_key.key_type() { assert!(d.is_none()); if let KeyType::EC { @@ -975,7 +1130,7 @@ mod tests { y: src_y, curve: src_curve, .. - } = &key.key + } = key.key_type() { assert_eq!(x, src_x); assert_eq!(y, src_y); @@ -995,20 +1150,20 @@ mod tests { let key = key.unwrap(); let public_key = key.to_public().unwrap(); - assert_eq!(key.kid, public_key.kid); - assert_eq!(public_key.operations, [KeyOperation::Verify].into()); + assert_eq!(key.kid(), public_key.kid()); + assert_eq!(public_key.operations(), &[KeyOperation::Verify].into()); assert!(public_key.encode.get().is_none()); assert!(public_key.decode.get().is_none()); - assert!(matches!(public_key.key, KeyType::OKP { .. })); + assert!(matches!(public_key.key_type(), KeyType::OKP { .. })); - if let KeyType::OKP { x, d, curve } = &public_key.key { + if let KeyType::OKP { x, d, curve } = public_key.key_type() { assert!(d.is_none()); if let KeyType::OKP { x: src_x, curve: src_curve, .. - } = &key.key + } = key.key_type() { assert_eq!(x, src_x); assert_eq!(curve, src_curve); @@ -1080,9 +1235,9 @@ mod tests { let json = serde_json::to_string(&key).unwrap(); let deserialized: Key = serde_json::from_str(&json).unwrap(); - assert_eq!(deserialized.algorithm, key.algorithm); - assert_eq!(deserialized.operations, key.operations); - assert_eq!(deserialized.kid, key.kid); + assert_eq!(deserialized.algorithm(), key.algorithm()); + assert_eq!(deserialized.operations(), key.operations()); + assert_eq!(deserialized.kid(), key.kid()); if let ( KeyType::OCT { @@ -1091,7 +1246,7 @@ mod tests { KeyType::OCT { secret: deserialized_secret, }, - ) = (&key.key, &deserialized.key) + ) = (key.key_type(), deserialized.key_type()) { assert_eq!(deserialized_secret, original_secret); } else { @@ -1104,16 +1259,16 @@ mod tests { let key = create_test_key(); let cloned = key.clone(); - assert_eq!(cloned.algorithm, key.algorithm); - assert_eq!(cloned.operations, key.operations); - assert_eq!(cloned.kid, key.kid); + assert_eq!(cloned.algorithm(), key.algorithm()); + assert_eq!(cloned.operations(), key.operations()); + assert_eq!(cloned.kid(), key.kid()); if let ( KeyType::OCT { secret: original_secret, }, KeyType::OCT { secret: cloned_secret }, - ) = (&key.key, &cloned.key) + ) = (key.key_type(), cloned.key_type()) { assert_eq!(cloned_secret, original_secret); } else { @@ -1240,26 +1395,23 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert!(key.operations.contains(&KeyOperation::Sign)); - assert!(key.operations.contains(&KeyOperation::Verify)); + assert!(key.operations().contains(&KeyOperation::Sign)); + assert!(key.operations().contains(&KeyOperation::Verify)); let public_key = key.to_public().unwrap(); - assert!(!public_key.operations.contains(&KeyOperation::Sign)); - assert!(public_key.operations.contains(&KeyOperation::Verify)); + assert!(!public_key.operations().contains(&KeyOperation::Sign)); + assert!(public_key.operations().contains(&KeyOperation::Verify)); - match key.key { - KeyType::RSA { - ref public, - ref private, - } => { + match key.key_type() { + KeyType::RSA { public, private } => { assert!(private.is_some()); assert_eq!(public.n.len(), 256); assert_eq!(public.e.len(), 3); - match public_key.key { + match public_key.key_type() { KeyType::RSA { - public: ref guest_public, - private: ref public_private, + public: guest_public, + private: public_private, } => { assert!(public_private.is_none()); assert_eq!(public.n, guest_public.n); @@ -1278,26 +1430,23 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert!(key.operations.contains(&KeyOperation::Sign)); - assert!(key.operations.contains(&KeyOperation::Verify)); + assert!(key.operations().contains(&KeyOperation::Sign)); + assert!(key.operations().contains(&KeyOperation::Verify)); let public_key = key.to_public().unwrap(); - assert!(!public_key.operations.contains(&KeyOperation::Sign)); - assert!(public_key.operations.contains(&KeyOperation::Verify)); + assert!(!public_key.operations().contains(&KeyOperation::Sign)); + assert!(public_key.operations().contains(&KeyOperation::Verify)); - match key.key { - KeyType::RSA { - ref public, - ref private, - } => { + match key.key_type() { + KeyType::RSA { public, private } => { assert!(private.is_some()); assert_eq!(public.n.len(), 256); assert_eq!(public.e.len(), 3); - match public_key.key { + match public_key.key_type() { KeyType::RSA { - public: ref guest_public, - private: ref public_private, + public: guest_public, + private: public_private, } => { assert!(public_private.is_none()); assert_eq!(public.n, guest_public.n); @@ -1331,7 +1480,7 @@ mod tests { if let KeyType::OCT { secret: original_secret, - } = &key.key + } = key.key_type() { assert_eq!(decoded, *original_secret); } else { @@ -1346,10 +1495,10 @@ mod tests { // Should be able to deserialize new format let key: Key = serde_json::from_str(unpadded_json).unwrap(); - assert_eq!(key.algorithm, Algorithm::HS256); - assert_eq!(key.kid, Some(crate::KeyId::decode("test-key-1").unwrap())); + assert_eq!(key.algorithm(), Algorithm::HS256); + assert_eq!(key.kid(), Some(&crate::KeyId::decode("test-key-1").unwrap())); - if let KeyType::OCT { secret } = &key.key { + if let KeyType::OCT { secret } = key.key_type() { assert_eq!(secret, b"test-secret-that-is-long-enough-for-hmac-sha256"); } else { panic!("Expected key to be OCT variant"); @@ -1363,10 +1512,10 @@ mod tests { // Should be able to deserialize old format for backwards compatibility let key: Key = serde_json::from_str(padded_json).unwrap(); - assert_eq!(key.algorithm, Algorithm::HS256); - assert_eq!(key.kid, Some(crate::KeyId::decode("test-key-1").unwrap())); + assert_eq!(key.algorithm(), Algorithm::HS256); + assert_eq!(key.kid(), Some(&crate::KeyId::decode("test-key-1").unwrap())); - if let KeyType::OCT { secret } = &key.key { + if let KeyType::OCT { secret } = key.key_type() { assert_eq!(secret, b"test-secret-that-is-long-enough-for-hmac-sha256"); } else { panic!("Expected key to be OCT variant"); @@ -1397,8 +1546,8 @@ mod tests { #[test] fn test_js_hs256_key_load() { let key = Key::from_str(JS_HS256_KEY).unwrap(); - assert_eq!(key.algorithm, Algorithm::HS256); - assert_eq!(key.kid, Some(crate::KeyId::decode("js-test-key").unwrap())); + assert_eq!(key.algorithm(), Algorithm::HS256); + assert_eq!(key.kid(), Some(&crate::KeyId::decode("js-test-key").unwrap())); } #[test] @@ -1428,11 +1577,11 @@ mod tests { #[test] fn test_js_eddsa_key_load() { let private_key = Key::from_str(JS_EDDSA_PRIVATE_KEY).unwrap(); - assert_eq!(private_key.algorithm, Algorithm::EdDSA); - assert!(matches!(private_key.key, KeyType::OKP { .. })); + assert_eq!(private_key.algorithm(), Algorithm::EdDSA); + assert!(matches!(private_key.key_type(), KeyType::OKP { .. })); let public_key = Key::from_str(JS_EDDSA_PUBLIC_KEY).unwrap(); - assert_eq!(public_key.algorithm, Algorithm::EdDSA); + assert_eq!(public_key.algorithm(), Algorithm::EdDSA); } #[test] @@ -1471,7 +1620,7 @@ mod tests { fn test_file_io_base64url() { let key = create_test_key(); let temp_dir = std::env::temp_dir(); - let temp_path = temp_dir.join("test_jwk.key"); + let temp_path = temp_dir.join("test_jwk.key_type()"); // Write key to file as base64url key.to_file(&temp_path).unwrap(); @@ -1493,16 +1642,16 @@ mod tests { // Read key back from file let loaded_key = Key::from_file(&temp_path).unwrap(); - assert_eq!(loaded_key.algorithm, key.algorithm); - assert_eq!(loaded_key.operations, key.operations); - assert_eq!(loaded_key.kid, key.kid); + assert_eq!(loaded_key.algorithm(), key.algorithm()); + assert_eq!(loaded_key.operations(), key.operations()); + assert_eq!(loaded_key.kid(), key.kid()); if let ( KeyType::OCT { secret: original_secret, }, KeyType::OCT { secret: loaded_secret }, - ) = (&key.key, &loaded_key.key) + ) = (key.key_type(), loaded_key.key_type()) { assert_eq!(loaded_secret, original_secret); } else { @@ -1517,7 +1666,7 @@ mod tests { fn test_file_io_raw_json() { let key = create_test_key(); let temp_dir = std::env::temp_dir(); - let temp_path = temp_dir.join("test_jwk_raw_json.key"); + let temp_path = temp_dir.join("test_jwk_raw_json.key_type()"); // Write key as raw JSON (backwards compat format) let json = serde_json::to_string(&key).unwrap(); @@ -1528,16 +1677,16 @@ mod tests { // Load via from_file (should auto-detect JSON) let loaded_key = Key::from_file(&temp_path).unwrap(); - assert_eq!(loaded_key.algorithm, key.algorithm); - assert_eq!(loaded_key.operations, key.operations); - assert_eq!(loaded_key.kid, key.kid); + assert_eq!(loaded_key.algorithm(), key.algorithm()); + assert_eq!(loaded_key.operations(), key.operations()); + assert_eq!(loaded_key.kid(), key.kid()); if let ( KeyType::OCT { secret: original_secret, }, KeyType::OCT { secret: loaded_secret }, - ) = (&key.key, &loaded_key.key) + ) = (key.key_type(), loaded_key.key_type()) { assert_eq!(loaded_secret, original_secret); } else { diff --git a/rs/moq-token/src/set.rs b/rs/moq-token/src/set.rs index 72ce969b19..933d085cd8 100644 --- a/rs/moq-token/src/set.rs +++ b/rs/moq-token/src/set.rs @@ -77,12 +77,20 @@ impl KeySet { }) } + /// Find the key with the given key ID. pub fn find_key(&self, kid: &str) -> Option> { - self.keys.iter().find(|k| k.kid.as_deref() == Some(kid)).cloned() + self.keys + .iter() + .find(|k| k.kid().is_some_and(|k| k.encode() == kid)) + .cloned() } + /// Find the first key that supports the given operation. pub fn find_supported_key(&self, operation: &KeyOperation) -> Option> { - self.keys.iter().find(|key| key.operations.contains(operation)).cloned() + self.keys + .iter() + .find(|key| key.operations().contains(operation)) + .cloned() } /// Sign the claims with the first key in the set that supports signing. @@ -166,7 +174,7 @@ mod tests { assert!(set.is_ok()); let set = set.unwrap(); assert_eq!(set.keys.len(), 1); - assert_eq!(set.keys[0].kid.as_deref(), Some("1")); + assert_eq!(set.keys[0].kid().map(|k| k.encode()), Some("1")); assert!(set.find_key("1").is_some()); } @@ -220,7 +228,7 @@ mod tests { let found = set.find_key("my-key"); assert!(found.is_some()); - assert_eq!(found.unwrap().kid.as_deref(), Some("my-key")); + assert_eq!(found.unwrap().kid().map(|k| k.encode()), Some("my-key")); } #[test] @@ -247,11 +255,8 @@ mod tests { #[test] fn test_find_supported_key() { - let mut sign_key = create_test_key(Some("sign")); - sign_key.operations = [KeyOperation::Sign].into(); - - let mut verify_key = create_test_key(Some("verify")); - verify_key.operations = [KeyOperation::Verify].into(); + let sign_key = create_test_key(Some("sign")).with_operations([KeyOperation::Sign]); + let verify_key = create_test_key(Some("verify")).with_operations([KeyOperation::Verify]); let set = KeySet { keys: vec![Arc::new(sign_key), Arc::new(verify_key)], @@ -259,11 +264,11 @@ mod tests { let found_sign = set.find_supported_key(&KeyOperation::Sign); assert!(found_sign.is_some()); - assert_eq!(found_sign.unwrap().kid.as_deref(), Some("sign")); + assert_eq!(found_sign.unwrap().kid().map(|k| k.encode()), Some("sign")); let found_verify = set.find_supported_key(&KeyOperation::Verify); assert!(found_verify.is_some()); - assert_eq!(found_verify.unwrap().kid.as_deref(), Some("verify")); + assert_eq!(found_verify.unwrap().kid().map(|k| k.encode()), Some("verify")); } #[test] @@ -279,9 +284,9 @@ mod tests { assert_eq!(public_set.keys.len(), 1); let public_key = &public_set.keys[0]; - assert_eq!(public_key.kid.as_deref(), Some("1")); - assert!(public_key.operations.contains(&KeyOperation::Verify)); - assert!(!public_key.operations.contains(&KeyOperation::Sign)); + assert_eq!(public_key.kid().map(|k| k.encode()), Some("1")); + assert!(public_key.operations().contains(&KeyOperation::Verify)); + assert!(!public_key.operations().contains(&KeyOperation::Sign)); } #[test] @@ -309,8 +314,7 @@ mod tests { #[test] fn test_encode_no_signing_key() { - let mut key = create_test_key(Some("1")); - key.operations = [KeyOperation::Verify].into(); + let key = create_test_key(Some("1")).with_operations([KeyOperation::Verify]); let set = KeySet { keys: vec![Arc::new(key)], }; @@ -413,7 +417,7 @@ mod tests { let loaded = KeySet::from_file(&path).expect("failed to read from file"); assert_eq!(loaded.keys.len(), 1); - assert_eq!(loaded.keys[0].kid.as_deref(), Some("1")); + assert_eq!(loaded.keys[0].kid().map(|k| k.encode()), Some("1")); // Clean up let _ = std::fs::remove_file(path); From bedf2ce3cf101760728967cbd87ced108d2e212d Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 21 Jul 2026 13:27:05 -0700 Subject: [PATCH 02/11] fix!: correct catalog, timeline, and teardown contracts found in API review A review of the accumulated pre-release API surface turned up several contracts worth fixing before they are published, plus one spec conformance bug. Container forward compatibility (the conformance bug). draft-lcurley-moq-hang says a consumer MUST ignore a rendition whose container `kind` it does not recognize. Neither implementation did: `hang::catalog::Container` had no fallback, so an unknown `kind` failed to deserialize the rendition and took the whole `Catalog` with it, and `js/hang` threw for the same reason. A publisher shipping a future container on one rendition would have blacked out every other rendition for all existing clients. Adds a `Container::Unknown` variant that preserves the original `kind` and fields verbatim so a relay or transcoder round-trips it, with the matching passthrough in `js/hang`. A malformed *known* kind still hard-errors rather than degrading to `Unknown`. The "ignore the rendition" policy lands in `moq_mux::select` and the watch decoders, and `moq-ffi` filters such renditions out so the language bindings never see one. The draft already specified this and needs no change. Catalog publishing is explicit. `moq_json::snapshot::Guard` and `moq_mux::catalog::Guard` published from `Drop` and discarded the result, so a serialization failure in a `CatalogExt` extension, or a write to a closed track, left the catalog absent or stale while the caller saw success. Adds `commit(self) -> Result<()>`; `Drop` still publishes for the one-liner form but now warns instead of swallowing. Also removes an `expect` reachable from that `Drop`, where a panic during unwind would have aborted the process. Timeline errors are real. `catalog::Producer::timeline` panicked via `expect` when its track name collided, through an ordinary public API and unreachable from `media_producer`, which could not propagate it; both now return `Result`. The consumer silently rewrote malformed input, turning an invalid timescale into milliseconds and an out-of-range PTS into zero, which sends seeking and live-edge logic to the wrong place; both are decode errors now. Drops the unused `RecordExt` parameter from the publishing side, where recording is crate private and always supplied the default. Teardown consumes the handle. `group::Producer::{finish,abort}` and `track::Producer::abort` took `&mut self`, leaving a usable-but-closed handle. `abort` now consumes `self` across moq-net and the wrapper types that own a producer. `finish` deliberately keeps `&mut self`: it declares completion and a later failure must still be reportable through `abort`, and on a track it only declares a final sequence while lower-numbered groups may still be written. Three `Drop` impls became RAII guards, since a type with `Drop` cannot have its fields moved out. Corrects the `moq_json::stream` module docs, which promised late joiners the retained suffix of the log. Past moq-net's 32 MiB group budget they get `Error::Lagged` and nothing, and a compressed suffix would be undecodable anyway because its DEFLATE window depends on the evicted prefix. The behavior is unchanged and intended; only the documentation was wrong. Co-Authored-By: Claude Opus 4.8 --- Cargo.lock | 1 + doc/concept/layer/hang.md | 3 + doc/lib/rs/env/native.md | 2 + js/hang/src/catalog/container.test.ts | 47 ++++++ js/hang/src/catalog/container.ts | 57 ++++++-- js/watch/src/audio/decoder.ts | 9 +- js/watch/src/video/decoder.ts | 12 +- rs/hang/src/catalog/container.rs | 141 ++++++++++++++++-- rs/hang/src/catalog/root.rs | 39 +++++ rs/libmoq/src/publish.rs | 4 +- rs/moq-audio/src/encode/producer.rs | 28 ++-- rs/moq-cli/src/publish.rs | 6 +- rs/moq-ffi/src/media.rs | 34 +++-- rs/moq-ffi/src/producer.rs | 4 +- rs/moq-gst/src/sink/pad.rs | 4 +- rs/moq-hls/src/export/mod.rs | 24 ++- rs/moq-hls/src/export/rendition.rs | 2 +- rs/moq-hls/src/import.rs | 15 +- rs/moq-json/Cargo.toml | 1 + rs/moq-json/src/snapshot.rs | 78 +++++++++- rs/moq-json/src/stream.rs | 10 +- rs/moq-mux/src/catalog/hang/container.rs | 15 ++ rs/moq-mux/src/catalog/hang/mod.rs | 1 + rs/moq-mux/src/catalog/producer.rs | 154 +++++++++++++++----- rs/moq-mux/src/catalog/tracks.rs | 4 +- rs/moq-mux/src/codec/aac/import.rs | 14 +- rs/moq-mux/src/codec/av1/import.rs | 12 +- rs/moq-mux/src/codec/flac/import.rs | 14 +- rs/moq-mux/src/codec/h264/import.rs | 22 +-- rs/moq-mux/src/codec/h265/import.rs | 12 +- rs/moq-mux/src/codec/legacy.rs | 14 +- rs/moq-mux/src/codec/mp3.rs | 14 +- rs/moq-mux/src/codec/opus/import.rs | 14 +- rs/moq-mux/src/codec/video.rs | 8 +- rs/moq-mux/src/codec/vp8/import.rs | 16 +- rs/moq-mux/src/codec/vp9/import.rs | 16 +- rs/moq-mux/src/container/consumer.rs | 2 +- rs/moq-mux/src/container/flv/export.rs | 4 + rs/moq-mux/src/container/flv/import.rs | 23 ++- rs/moq-mux/src/container/flv/import_test.rs | 2 - rs/moq-mux/src/container/fmp4/export.rs | 32 +++- rs/moq-mux/src/container/fmp4/import.rs | 39 ++--- rs/moq-mux/src/container/fmp4/muxer.rs | 5 +- rs/moq-mux/src/container/mkv/export.rs | 1 + rs/moq-mux/src/container/mkv/import.rs | 22 ++- rs/moq-mux/src/container/producer.rs | 6 +- rs/moq-mux/src/container/ts/export.rs | 4 + rs/moq-mux/src/container/ts/import.rs | 86 ++++++----- rs/moq-mux/src/error.rs | 17 +++ rs/moq-mux/src/import/container.rs | 10 +- rs/moq-mux/src/import/track.rs | 62 ++++---- rs/moq-mux/src/select.rs | 21 ++- rs/moq-mux/src/timeline.rs | 111 ++++++++++---- rs/moq-net/src/model/broadcast.rs | 2 +- rs/moq-net/src/model/group.rs | 17 ++- rs/moq-net/src/model/origin.rs | 2 +- rs/moq-net/src/model/track.rs | 35 +++-- rs/moq-rtc/src/codec/av1.rs | 4 +- rs/moq-rtc/src/codec/h264.rs | 4 +- rs/moq-rtc/src/codec/h265.rs | 4 +- rs/moq-rtc/src/codec/mod.rs | 19 ++- rs/moq-rtc/src/codec/opus.rs | 4 +- rs/moq-rtc/src/codec/vp8.rs | 29 ++-- rs/moq-rtc/src/codec/vp9.rs | 29 ++-- rs/moq-rtc/src/session.rs | 4 +- rs/moq-rtmp/src/dial.rs | 4 +- rs/moq-rtmp/src/server.rs | 4 +- rs/moq-srt/src/ts.rs | 4 +- rs/moq-transcode/src/rung.rs | 22 +-- rs/moq-video/src/encode/producer.rs | 8 +- 70 files changed, 1046 insertions(+), 447 deletions(-) create mode 100644 js/hang/src/catalog/container.test.ts diff --git a/Cargo.lock b/Cargo.lock index 428eeee6a0..69121d07ac 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4353,6 +4353,7 @@ dependencies = [ "serde", "serde_json", "thiserror 2.0.18", + "tracing", ] [[package]] diff --git a/doc/concept/layer/hang.md b/doc/concept/layer/hang.md index b905cefb0a..8c00feecb4 100644 --- a/doc/concept/layer/hang.md +++ b/doc/concept/layer/hang.md @@ -125,6 +125,9 @@ Unfortunately, the raw codec bitstream lacks timestamp information so we need so Containers can support additional features and configuration. For example, `CMAF` specifies a timescale instead of hard-coding it to microseconds like `legacy`. +The `kind` field selects the framing and new kinds can be added over time. +A consumer ignores any rendition whose `kind` it doesn't recognize, keeping the rest of the catalog usable, and carries the unrecognized entry through untouched when it republishes the catalog. + ### Legacy This is a lightweight container with no frills attached. diff --git a/doc/lib/rs/env/native.md b/doc/lib/rs/env/native.md index 165c7834fe..4cf8381070 100644 --- a/doc/lib/rs/env/native.md +++ b/doc/lib/rs/env/native.md @@ -188,6 +188,8 @@ Check the `container` field for each rendition: `OrderedConsumer` decodes legacy timestamps for you automatically. +Anything else decodes as `Container::Unknown`, which preserves the original JSON so you can republish the catalog unchanged. Skip those renditions: their frames can't be parsed. + ## Next Steps - [hang format](/concept/layer/hang) — Catalog schema and container details diff --git a/js/hang/src/catalog/container.test.ts b/js/hang/src/catalog/container.test.ts new file mode 100644 index 0000000000..84124d9c19 --- /dev/null +++ b/js/hang/src/catalog/container.test.ts @@ -0,0 +1,47 @@ +import { expect, test } from "bun:test"; +import { ContainerSchema, containerSupported } from "./container.ts"; +import { RootSchema } from "./root.ts"; + +test("known containers round-trip", () => { + for (const container of [{ kind: "legacy" }, { kind: "cmaf", init: "AAEC" }, { kind: "loc" }]) { + const parsed = ContainerSchema.parse(container); + expect(parsed).toEqual(container); + expect(containerSupported(parsed)).toBe(true); + } +}); + +test("unknown container is preserved instead of throwing", () => { + const container = { kind: "future", extra: { nested: [1, 2] }, flag: true }; + const parsed = ContainerSchema.parse(container); + expect(parsed).toEqual(container); + expect(containerSupported(parsed)).toBe(false); +}); + +test("catalog with an unknown container keeps its other renditions", () => { + const catalog = { + video: { + renditions: { + future: { + codec: "avc1.64001f", + container: { kind: "future", magic: 7 }, + }, + legacy: { + codec: "avc1.64001f", + codedWidth: 1280, + codedHeight: 720, + container: { kind: "legacy" }, + }, + }, + }, + }; + + const parsed = RootSchema.parse(catalog); + if (!parsed.video || !("renditions" in parsed.video)) throw new Error("missing video section"); + + const known = parsed.video.renditions.legacy; + expect(known?.container.kind).toBe("legacy"); + expect(Number(known?.codedWidth)).toBe(1280); + + // The unknown rendition survives a republish intact. + expect(parsed.video.renditions.future?.container).toEqual({ kind: "future", magic: 7 }); +}); diff --git a/js/hang/src/catalog/container.ts b/js/hang/src/catalog/container.ts index 13d7c64edc..05f7886d15 100644 --- a/js/hang/src/catalog/container.ts +++ b/js/hang/src/catalog/container.ts @@ -1,5 +1,18 @@ import * as z from "zod/mini"; +/** The container kinds this build knows how to decode. */ +const KNOWN_KINDS = ["legacy", "cmaf", "loc"]; + +/** + * A container this build does not recognize, preserved verbatim. + * + * Kept intact so reparsing and republishing a catalog round-trips the rendition instead of + * corrupting it. Such a rendition must be ignored rather than decoded. + */ +export const UnknownContainerSchema = z.looseObject({ + kind: z.string(), +}); + /** * Container format for frame timestamp encoding and frame payload structure. * @@ -9,21 +22,45 @@ import * as z from "zod/mini"; * The init segment (ftyp+moov) is base64-encoded in the catalog. * - "loc": Low Overhead Container (draft-ietf-moq-loc). Each frame has a small * property block followed by the codec payload. + * + * Anything else parses as {@link UnknownContainerSchema} instead of throwing, so one rendition + * using a future container does not take down the rest of the catalog. */ export const ContainerSchema = z._default( - z.discriminatedUnion("kind", [ - // The default hang container - z.object({ kind: z.literal("legacy") }), - // CMAF container with base64-encoded init segment (ftyp+moov). - z.object({ - kind: z.literal("cmaf"), - init: z.base64(), - }), - // Low Overhead Container. - z.object({ kind: z.literal("loc") }), + z.union([ + z.discriminatedUnion("kind", [ + // The default hang container + z.object({ kind: z.literal("legacy") }), + // CMAF container with base64-encoded init segment (ftyp+moov). + z.object({ + kind: z.literal("cmaf"), + init: z.base64(), + }), + // Low Overhead Container. + z.object({ kind: z.literal("loc") }), + ]), + UnknownContainerSchema, ]), { kind: "legacy" }, ); /** The per-frame container format declared in the catalog. */ export type Container = z.infer; + +/** The CMAF variant of {@link Container}, carrying the base64 init segment. */ +export type CmafContainer = Extract; + +/** + * Whether the container is CMAF, narrowing it so `init` is available. + * + * The passthrough case makes `kind` a plain string, so an equality check alone no longer + * narrows the union. + */ +export function isCmafContainer(container: Container): container is CmafContainer { + return container.kind === "cmaf"; +} + +/** Whether a container can be decoded by this build, i.e. its `kind` is recognized. */ +export function containerSupported(container: Container): boolean { + return KNOWN_KINDS.includes(container.kind); +} diff --git a/js/watch/src/audio/decoder.ts b/js/watch/src/audio/decoder.ts index 1d52f3e96a..e8a53d9c50 100644 --- a/js/watch/src/audio/decoder.ts +++ b/js/watch/src/audio/decoder.ts @@ -332,7 +332,7 @@ export class Decoder { } #runCmafDecoder(effect: Effect, sub: Moq.Track.Subscriber, config: Catalog.AudioConfig): void { - if (config.container.kind !== "cmaf") return; // just to help typescript + if (!Catalog.isCmafContainer(config.container)) return; // just to help typescript const initSegment = base64ToBytes(config.container.init); const init = Container.Cmaf.decodeInitSegment(initSegment); @@ -513,6 +513,11 @@ export class Decoder { } async function supported(config: Catalog.AudioConfig): Promise { + if (!Catalog.containerSupported(config.container)) { + console.warn(`audio: ignoring rendition with unknown container: ${config.container.kind}`); + return false; + } + // Opus only runs at its native rates, so a catalog advertising anything else is wrong and Safari // refuses to decode it. Warn rather than reject: Chrome and Firefox ignore the configured rate and // play these streams fine, so rejecting would silence them for a publisher they handle today. @@ -525,7 +530,7 @@ async function supported(config: Catalog.AudioConfig): Promise { if (config.codec !== "opus") { if (config.description) { description = Util.Hex.toBytes(config.description); - } else if (config.container.kind === "cmaf") { + } else if (Catalog.isCmafContainer(config.container)) { try { description = Container.Cmaf.decodeInitSegment(base64ToBytes(config.container.init)).description; } catch (err) { diff --git a/js/watch/src/video/decoder.ts b/js/watch/src/video/decoder.ts index 3574a82b47..ef7f8eed82 100644 --- a/js/watch/src/video/decoder.ts +++ b/js/watch/src/video/decoder.ts @@ -413,9 +413,10 @@ class DecoderTrack { } #runCmaf(effect: Effect, sub: Moq.Track.Subscriber, decoder: VideoDecoder): void { - if (this.config.container.kind !== "cmaf") return; + const container = this.config.container; + if (!Catalog.isCmafContainer(container)) return; - const initSegment = base64ToBytes(this.config.container.init); + const initSegment = base64ToBytes(container.init); const init = Container.Cmaf.decodeInitSegment(initSegment); const description = this.config.description ? Util.Hex.toBytes(this.config.description) : init.description; @@ -555,10 +556,15 @@ class DecoderTrack { } async function supported(config: Catalog.VideoConfig): Promise { + if (!Catalog.containerSupported(config.container)) { + console.warn(`video: ignoring rendition with unknown container: ${config.container.kind}`); + return false; + } + let description: Uint8Array | undefined; if (config.description) { description = Util.Hex.toBytes(config.description); - } else if (config.container.kind === "cmaf") { + } else if (Catalog.isCmafContainer(config.container)) { try { description = Container.Cmaf.decodeInitSegment(base64ToBytes(config.container.init)).description; } catch (err) { diff --git a/rs/hang/src/catalog/container.rs b/rs/hang/src/catalog/container.rs index 9e5cc227b2..126f42601a 100644 --- a/rs/hang/src/catalog/container.rs +++ b/rs/hang/src/catalog/container.rs @@ -1,47 +1,160 @@ use bytes::Bytes; -use serde::{Deserialize, Serialize}; +use serde::{Deserialize, Deserializer, Serialize, Serializer, de}; use serde_with::{base64::Base64, serde_as}; /// Container format for frame timestamp encoding and frame payload structure. /// -/// - "legacy": QUIC VarInt timestamp prefix followed by the raw codec payload. -/// Timestamps are in microseconds. -/// - "cmaf": Fragmented MP4 - frames contain complete moof+mdat fragments. The -/// init segment (ftyp+moov) is base64-encoded in the catalog. -/// - "loc": Low Overhead Container (draft-ietf-moq-loc). Each frame is a small -/// property block followed by the codec payload. -/// /// JSON examples: /// ```json /// { "kind": "cmaf", "init": "" } /// { "kind": "loc" } /// ``` +/// +/// An unrecognized `kind` decodes to [`Container::Unknown`] instead of failing, so one +/// rendition using a future container does not take down the rest of the catalog. Such a +/// rendition must be ignored by consumers. +#[derive(Debug, Clone, PartialEq, Default)] +pub enum Container { + /// A QUIC VarInt timestamp prefix followed by the raw codec payload. + /// Timestamps are in microseconds. + #[default] + Legacy, + + /// Fragmented MP4: each frame is a complete moof+mdat fragment. + Cmaf { + /// CMAF init segment (ftyp+moov). Encoded as base64 over the wire. + init: Bytes, + }, + + /// Low Overhead Container (draft-ietf-moq-loc): each frame is a small + /// property block followed by the codec payload. + Loc, + + /// A container this build does not recognize, preserved verbatim. + Unknown(UnknownContainer), +} + +/// The raw JSON of a container whose `kind` is not recognized. +/// +/// Kept intact so a relay or transcoder that reparses and republishes a catalog round-trips +/// the rendition byte-for-byte rather than corrupting it. +#[derive(Debug, Clone, PartialEq)] +pub struct UnknownContainer(serde_json::Map); + +impl UnknownContainer { + /// The `kind` as it appeared on the wire, or `None` if it was absent or not a string. + pub fn kind(&self) -> Option<&str> { + self.0.get("kind").and_then(serde_json::Value::as_str) + } + + /// The full JSON object, including `kind`. + pub fn fields(&self) -> &serde_json::Map { + &self.0 + } +} + +/// The containers this build knows how to encode and decode. +/// +/// Split out so the tagged representation stays derived while [`Container`] keeps a catch-all. #[serde_as] -#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Default)] +#[derive(Serialize, Deserialize)] #[serde(rename_all = "camelCase")] #[serde(tag = "kind")] -pub enum Container { +enum Known { #[serde(rename = "legacy")] - #[default] Legacy, Cmaf { - /// CMAF init segment (ftyp+moov). Encoded as base64 over the wire. #[serde_as(as = "Base64")] init: Bytes, }, Loc, } +impl Serialize for Container { + fn serialize(&self, serializer: S) -> Result { + let known = match self { + Self::Legacy => Known::Legacy, + // Bytes is refcounted, so this clone is cheap. + Self::Cmaf { init } => Known::Cmaf { init: init.clone() }, + Self::Loc => Known::Loc, + Self::Unknown(unknown) => return unknown.0.serialize(serializer), + }; + + known.serialize(serializer) + } +} + +impl<'de> Deserialize<'de> for Container { + fn deserialize>(deserializer: D) -> Result { + let object = serde_json::Map::deserialize(deserializer)?; + + // Only route to the derived enum for kinds we know, so a malformed known container is + // still a hard error instead of silently becoming Unknown. + match object.get("kind").and_then(serde_json::Value::as_str) { + Some("legacy" | "cmaf" | "loc") => { + let known = Known::deserialize(serde_json::Value::Object(object)).map_err(de::Error::custom)?; + Ok(match known { + Known::Legacy => Self::Legacy, + Known::Cmaf { init } => Self::Cmaf { init }, + Known::Loc => Self::Loc, + }) + } + _ => Ok(Self::Unknown(UnknownContainer(object))), + } + } +} + #[cfg(test)] mod tests { use super::*; + #[test] + fn legacy_roundtrip() { + let parsed: Container = serde_json::from_str(r#"{"kind":"legacy"}"#).unwrap(); + assert_eq!(parsed, Container::Legacy); + assert_eq!(serde_json::to_string(&parsed).unwrap(), r#"{"kind":"legacy"}"#); + } + + #[test] + fn cmaf_roundtrip() { + let parsed: Container = serde_json::from_str(r#"{"kind":"cmaf","init":"AAEC"}"#).unwrap(); + assert_eq!( + parsed, + Container::Cmaf { + init: Bytes::from_static(&[0, 1, 2]) + } + ); + assert_eq!( + serde_json::to_string(&parsed).unwrap(), + r#"{"kind":"cmaf","init":"AAEC"}"# + ); + } + #[test] fn loc_roundtrip() { let parsed: Container = serde_json::from_str(r#"{"kind":"loc"}"#).unwrap(); assert_eq!(parsed, Container::Loc); + assert_eq!(serde_json::to_string(&parsed).unwrap(), r#"{"kind":"loc"}"#); + } - let json = serde_json::to_string(&parsed).unwrap(); - assert_eq!(json, r#"{"kind":"loc"}"#); + #[test] + fn unknown_roundtrip() { + // Keys are sorted because serde_json::Map is a BTreeMap by default. + let json = r#"{"extra":{"nested":[1,2]},"flag":true,"kind":"future"}"#; + let parsed: Container = serde_json::from_str(json).unwrap(); + + let Container::Unknown(unknown) = &parsed else { + panic!("expected unknown: {parsed:?}"); + }; + assert_eq!(unknown.kind(), Some("future")); + assert_eq!(unknown.fields().len(), 3); + + assert_eq!(serde_json::to_string(&parsed).unwrap(), json); + } + + #[test] + fn malformed_known_kind_errors() { + // cmaf without init is not a valid cmaf container and must not degrade to Unknown. + serde_json::from_str::(r#"{"kind":"cmaf"}"#).unwrap_err(); } } diff --git a/rs/hang/src/catalog/root.rs b/rs/hang/src/catalog/root.rs index 4952e5ba16..7a0708635a 100644 --- a/rs/hang/src/catalog/root.rs +++ b/rs/hang/src/catalog/root.rs @@ -338,6 +338,45 @@ mod test { ); } + #[test] + fn unknown_container_keeps_siblings() { + // A rendition using a future container must not take down the rest of the catalog. + let encoded = r#"{ + "video": { + "renditions": { + "future": { + "codec": "avc1.64001f", + "container": {"kind": "future", "magic": 7} + }, + "legacy": { + "codec": "avc1.64001f", + "codedWidth": 1280, + "codedHeight": 720, + "container": {"kind": "legacy"} + } + } + } + }"#; + + let parsed = Catalog::from_str(encoded).expect("failed to decode"); + + let known = parsed.video.renditions.get("legacy").expect("missing rendition"); + assert_eq!(known.container, Container::Legacy); + assert_eq!(known.coded_width, Some(1280)); + + let future = parsed.video.renditions.get("future").expect("missing rendition"); + let Container::Unknown(unknown) = &future.container else { + panic!("expected unknown container: {:?}", future.container); + }; + assert_eq!(unknown.kind(), Some("future")); + + // The unknown rendition survives a republish intact. + let output = parsed.to_json().expect("failed to encode"); + let reparsed = Catalog::from_str(&output).expect("failed to re-decode"); + assert_eq!(parsed, reparsed, "re-encoded catalog did not round-trip"); + assert!(output.contains(r#""magic":7"#), "unknown fields dropped: {output}"); + } + #[test] fn extension_roundtrip() { // An application extends the catalog with its own root section by flattening Catalog. diff --git a/rs/libmoq/src/publish.rs b/rs/libmoq/src/publish.rs index 20a87468fe..26ec8d010e 100644 --- a/rs/libmoq/src/publish.rs +++ b/rs/libmoq/src/publish.rs @@ -245,7 +245,7 @@ impl Publish { /// Abort a raw track with an application error code. pub fn track_abort(&mut self, track: Id, error_code: u16) -> Result<(), Error> { - let mut track = self.tracks.remove(track).ok_or(Error::TrackNotFound)?; + let track = self.tracks.remove(track).ok_or(Error::TrackNotFound)?; track.abort(moq_net::Error::App(error_code))?; Ok(()) } @@ -326,7 +326,7 @@ impl Publish { /// Abort a raw group with an application error code. pub fn group_abort(&mut self, group: Id, error_code: u16) -> Result<(), Error> { - let mut group = self.groups.remove(group).ok_or(Error::GroupNotFound)?; + let group = self.groups.remove(group).ok_or(Error::GroupNotFound)?; group.abort(moq_net::Error::App(error_code))?; Ok(()) } diff --git a/rs/moq-audio/src/encode/producer.rs b/rs/moq-audio/src/encode/producer.rs index 377c255001..e67a4e7c7f 100644 --- a/rs/moq-audio/src/encode/producer.rs +++ b/rs/moq-audio/src/encode/producer.rs @@ -81,8 +81,8 @@ pub struct Producer { encoder: Encoder, resampler: Option, track: moq_mux::container::Producer, - track_name: String, - catalog: moq_mux::catalog::Producer, + /// Owns the catalog rendition, retiring it when this producer goes away. + rendition: Rendition, pending: Vec, /// Samples emitted since the current epoch (reset by [`reset_epoch`](Self::reset_epoch)). frames_produced: u64, @@ -129,19 +129,18 @@ impl Producer { None => moq_mux::import::unique_track(broadcast, &format!(".{}", options.codec))?, }; let name = track.name().to_string(); - let track = catalog.media_producer(track, moq_mux::container::legacy::Wire); + let track = catalog.media_producer(track, moq_mux::container::legacy::Wire)?; let mut catalog_mut = catalog.clone(); let mut config = encoder.catalog(); - config.timeline = Some(catalog.timeline(&name).section()); + config.timeline = Some(catalog.timeline(&name)?.section()); catalog_mut.lock().audio.insert(&name, config)?; Ok(Self { encoder, resampler, track, - track_name: name, - catalog, + rendition: Rendition { catalog, name }, pending: Vec::new(), frames_produced: 0, epoch_us: None, @@ -150,7 +149,7 @@ impl Producer { /// The name of the published track, which is [`Options::track`] resolved. pub fn track_name(&self) -> &str { - &self.track_name + &self.rendition.name } /// The underlying track producer, e.g. to watch subscriber state via @@ -249,14 +248,23 @@ impl Producer { /// Abort the track with `err` instead of finishing it, so subscribers see the /// real cause rather than [`moq_net::Error::Dropped`]. Pending samples are dropped. - pub fn abort(mut self, err: moq_net::Error) { + pub fn abort(self, err: moq_net::Error) { self.track.abort(err); } } -impl Drop for Producer { +/// The producer's catalog entry, removed however the producer ends. +/// +/// A separate value rather than a `Drop` on [`Producer`] itself, so the terminal +/// [`finish`](Producer::finish) / [`abort`](Producer::abort) can consume the track. +struct Rendition { + catalog: moq_mux::catalog::Producer, + name: String, +} + +impl Drop for Rendition { fn drop(&mut self) { - self.catalog.lock().audio.remove(&self.track_name); + self.catalog.lock().audio.remove(&self.name); } } diff --git a/rs/moq-cli/src/publish.rs b/rs/moq-cli/src/publish.rs index a51d94ba80..a7f39472ce 100644 --- a/rs/moq-cli/src/publish.rs +++ b/rs/moq-cli/src/publish.rs @@ -175,8 +175,8 @@ impl PublishDecoder { } /// Abort the tracks with `err` instead of finishing, so subscribers see the - /// real cause rather than `Error::Dropped`. - fn abort(&mut self, err: moq_net::Error) { + /// real cause rather than `Error::Dropped`. Consumes the decoder. + fn abort(self, err: moq_net::Error) { match self { Self::Avc3 { import, .. } => import.abort(err), Self::Fmp4(d) => d.abort(err), @@ -238,7 +238,7 @@ impl Publish { let source = match format { PublishFormat::Avc3 => { let track = moq_mux::import::unique_track(&mut broadcast, ".avc3")?; - let import = moq_mux::codec::h264::Import::new(track, catalog.reserve(), Default::default()); + let import = moq_mux::codec::h264::Import::new(track, catalog.reserve(), Default::default())?; let split = Box::new(moq_mux::codec::h264::Split::new()); Source::Stream(PublishDecoder::Avc3 { split, diff --git a/rs/moq-ffi/src/media.rs b/rs/moq-ffi/src/media.rs index 2541b07ae4..c22919f75c 100644 --- a/rs/moq-ffi/src/media.rs +++ b/rs/moq-ffi/src/media.rs @@ -17,12 +17,20 @@ pub enum MoqContainer { Loc, } -impl From for MoqContainer { - fn from(container: hang::catalog::Container) -> Self { +impl MoqContainer { + /// Convert a catalog container, or `None` if its `kind` is not recognized. + /// + /// A rendition we can't parse is dropped from the catalog we hand to bindings, per the + /// hang spec: a consumer must ignore a rendition whose container it doesn't recognize. + fn from_catalog(container: &hang::catalog::Container) -> Option { match container { - hang::catalog::Container::Legacy => Self::Legacy, - hang::catalog::Container::Cmaf { init, .. } => Self::Cmaf { init: init.to_vec() }, - hang::catalog::Container::Loc => Self::Loc, + hang::catalog::Container::Legacy => Some(Self::Legacy), + hang::catalog::Container::Cmaf { init, .. } => Some(Self::Cmaf { init: init.to_vec() }), + hang::catalog::Container::Loc => Some(Self::Loc), + hang::catalog::Container::Unknown(unknown) => { + tracing::warn!(kind = unknown.kind(), "ignoring unknown container"); + None + } } } } @@ -176,8 +184,8 @@ pub(crate) fn convert_catalog(catalog: &moq_mux::catalog::hang::Catalog Result<(), MoqError> { let _guard = crate::ffi::RUNTIME.enter(); let mut guard = self.inner.lock().unwrap(); - let mut track = guard.take().ok_or(MoqError::Closed)?; + let track = guard.take().ok_or(MoqError::Closed)?; track.abort(moq_net::Error::App(error_code))?; Ok(()) } @@ -824,7 +824,7 @@ impl MoqGroupProducer { pub fn abort(&self, error_code: u16) -> Result<(), MoqError> { let _guard = crate::ffi::RUNTIME.enter(); let mut guard = self.inner.lock().unwrap(); - let mut group = guard.take().ok_or(MoqError::Closed)?; + let group = guard.take().ok_or(MoqError::Closed)?; group.abort(moq_net::Error::App(error_code))?; Ok(()) } diff --git a/rs/moq-gst/src/sink/pad.rs b/rs/moq-gst/src/sink/pad.rs index 8757680f2b..8b9c330a3c 100644 --- a/rs/moq-gst/src/sink/pad.rs +++ b/rs/moq-gst/src/sink/pad.rs @@ -118,7 +118,7 @@ impl Pad { let name = broadcast.unique_name(".mp3"); let request = broadcast.reserve_track(name)?; let producer = request.accept(hang::container::track_info()); - moq_mux::codec::mp3::Import::new(producer, catalog.reserve(), config.into()).into() + moq_mux::codec::mp3::Import::new(producer, catalog.reserve(), config.into())?.into() } "audio/mpeg" => { // AAC: the AudioSpecificConfig rides in caps as codec_data, not in the bitstream. @@ -149,7 +149,7 @@ impl Pad { let name = broadcast.unique_name(".opus"); let request = broadcast.reserve_track(name)?; let producer = request.accept(hang::container::track_info()); - moq_mux::codec::opus::Import::new(producer, catalog.reserve(), config.into()).into() + moq_mux::codec::opus::Import::new(producer, catalog.reserve(), config.into())?.into() } other => anyhow::bail!("unsupported caps: {other}"), }; diff --git a/rs/moq-hls/src/export/mod.rs b/rs/moq-hls/src/export/mod.rs index 71252de4e3..a48d422b3f 100644 --- a/rs/moq-hls/src/export/mod.rs +++ b/rs/moq-hls/src/export/mod.rs @@ -233,13 +233,15 @@ mod tests { let mut registration = reserved.video("video0"); let mut config = hang::catalog::VideoConfig::new(hang::catalog::VideoCodec::VP8); config.framerate = Some(30.0); - config.timeline = Some(catalog.timeline("video0").section()); + config.timeline = Some(catalog.timeline("video0").unwrap().section()); registration.set(config); drop(reserved); // Three GOPs, 2s apart: groups 0 and 1 are complete, group 2 is the live edge. let track = broadcast.create_track("video0", None).unwrap(); - let mut media = catalog.media_producer(track, moq_mux::catalog::hang::Container::Legacy); + let mut media = catalog + .media_producer(track, moq_mux::catalog::hang::Container::Legacy) + .unwrap(); media.write(frame(0, true)).unwrap(); media.write(frame(1_000_000, false)).unwrap(); media.write(frame(2_000_000, true)).unwrap(); @@ -302,14 +304,16 @@ mod tests { let mut registration = reserved.video("video0"); let mut config = hang::catalog::VideoConfig::new(hang::catalog::VideoCodec::VP8); config.framerate = Some(30.0); - config.timeline = Some(catalog.timeline("video0").section()); + config.timeline = Some(catalog.timeline("video0").unwrap().section()); registration.set(config); drop(reserved); // Two GOPs: group 0 is complete, while group 1 stays at the live edge until the // publisher finishes. let track = broadcast.create_track("video0", None).unwrap(); - let mut media = catalog.media_producer(track, moq_mux::catalog::hang::Container::Legacy); + let mut media = catalog + .media_producer(track, moq_mux::catalog::hang::Container::Legacy) + .unwrap(); media.write(frame(0, true)).unwrap(); media.write(frame(2_000_000, true)).unwrap(); @@ -395,12 +399,14 @@ mod tests { let mut registration = reserved.video("video0"); let mut config = hang::catalog::VideoConfig::new(hang::catalog::VideoCodec::VP8); config.framerate = Some(30.0); - config.timeline = Some(catalog.timeline("video0").section()); + config.timeline = Some(catalog.timeline("video0").unwrap().section()); registration.set(config); drop(reserved); let track = broadcast.create_track("video0", None).unwrap(); - let mut media = catalog.media_producer(track, moq_mux::catalog::hang::Container::Legacy); + let mut media = catalog + .media_producer(track, moq_mux::catalog::hang::Container::Legacy) + .unwrap(); media.write(frame(0, true)).unwrap(); media.write(frame(2_000_000, true)).unwrap(); @@ -452,13 +458,15 @@ mod tests { let mut registration = reserved.video("video0"); let mut config = hang::catalog::VideoConfig::new(hang::catalog::VideoCodec::VP8); config.framerate = Some(30.0); - config.timeline = Some(catalog.timeline("video0").section()); + config.timeline = Some(catalog.timeline("video0").unwrap().section()); registration.set(config); drop(reserved); // Groups 0 and 1 are complete; group 2 is the live edge until the publisher drops. let track = broadcast.create_track("video0", None).unwrap(); - let mut media = catalog.media_producer(track, moq_mux::catalog::hang::Container::Legacy); + let mut media = catalog + .media_producer(track, moq_mux::catalog::hang::Container::Legacy) + .unwrap(); media.write(frame(0, true)).unwrap(); media.write(frame(2_000_000, true)).unwrap(); media.write(frame(4_000_000, true)).unwrap(); diff --git a/rs/moq-hls/src/export/rendition.rs b/rs/moq-hls/src/export/rendition.rs index eb45f4423b..96c3efca1e 100644 --- a/rs/moq-hls/src/export/rendition.rs +++ b/rs/moq-hls/src/export/rendition.rs @@ -444,7 +444,7 @@ async fn watch( let _ = live.broadcast.set(broadcast.clone()); let mut timeline = moq_mux::timeline::Consumer::<()>::subscribe(&broadcast, section).await?; - while let Some(entry) = timeline.next().await.map_err(moq_mux::Error::from)? { + while let Some(entry) = timeline.next().await? { live.push(entry, window); } Ok(()) diff --git a/rs/moq-hls/src/import.rs b/rs/moq-hls/src/import.rs index f415800248..8aef546c66 100644 --- a/rs/moq-hls/src/import.rs +++ b/rs/moq-hls/src/import.rs @@ -617,17 +617,18 @@ impl Import { /// Abort every rendition's importer with `err` so subscribers see the real cause. /// /// Call this when the import is torn down with a known error; simply dropping the - /// [`Import`] instead lets its tracks end as [`moq_net::Error::Dropped`]. - pub fn abort(&mut self, err: moq_net::Error) { + /// [`Import`] instead lets its tracks end as [`moq_net::Error::Dropped`]. Consumes + /// the import: every rendition is dead afterwards. + pub fn abort(mut self, err: moq_net::Error) { for track in &mut self.video { - if let Some(importer) = &mut track.importer { + if let Some(importer) = track.importer.take() { importer.abort(err.clone()); } } - if let Some(track) = &mut self.audio { - if let Some(importer) = &mut track.importer { - importer.abort(err.clone()); - } + if let Some(track) = &mut self.audio + && let Some(importer) = track.importer.take() + { + importer.abort(err.clone()); } } diff --git a/rs/moq-json/Cargo.toml b/rs/moq-json/Cargo.toml index 8f7e1a7f80..cf813ad4e3 100644 --- a/rs/moq-json/Cargo.toml +++ b/rs/moq-json/Cargo.toml @@ -21,6 +21,7 @@ moq-net = { workspace = true } serde = { workspace = true } serde_json = "1" thiserror = "2" +tracing = "0.1" [dev-dependencies] criterion = "0.8" diff --git a/rs/moq-json/src/snapshot.rs b/rs/moq-json/src/snapshot.rs index bf775a778f..29beb6fb70 100644 --- a/rs/moq-json/src/snapshot.rs +++ b/rs/moq-json/src/snapshot.rs @@ -168,6 +168,9 @@ impl Producer { /// producer's lock for its lifetime, so independent owners are serialized: each one starts from /// the latest value and their changes compose instead of clobbering. Don't hold a guard across /// an `.await`, since that keeps the lock held while suspended. + /// + /// Publishing on drop can fail (a closed track, a value that won't serialize) and only logs a + /// warning. Call [`Guard::commit`] instead to handle the error. pub fn lock(&mut self) -> Guard<'_, T> where T: Default + DeserializeOwned, @@ -196,12 +199,36 @@ impl Producer { /// /// Holds the producer's lock for its lifetime and derefs to the current value. Mutating it through /// [`DerefMut`] marks it dirty, and dropping a dirty guard publishes the edited value. +/// +/// Publishing on drop swallows any error into a warning, so prefer [`commit`](Self::commit) when the +/// caller can act on a failure. pub struct Guard<'a, T: Serialize> { inner: MutexGuard<'a, Inner>, value: T, dirty: bool, } +impl Guard<'_, T> { + /// Publish the edited value, returning any error. + /// + /// Consumes the guard, so the subsequent drop publishes nothing. A no-op if the value was never + /// mutated. + pub fn commit(mut self) -> Result<()> { + self.publish() + } + + /// Publish a dirty value once, clearing the dirty flag so it isn't published again. + fn publish(&mut self) -> Result<()> { + if !self.dirty { + return Ok(()); + } + self.dirty = false; + + // We already hold the lock, so publish through the held guard rather than re-locking. + self.inner.update(&self.value) + } +} + impl Deref for Guard<'_, T> { type Target = T; @@ -219,12 +246,9 @@ impl DerefMut for Guard<'_, T> { impl Drop for Guard<'_, T> { fn drop(&mut self) { - if !self.dirty { - return; + if let Err(err) = self.publish() { + tracing::warn!(%err, "failed to publish JSON value on guard drop"); } - - // We already hold the lock, so publish through the held guard rather than re-locking. - let _ = self.inner.update(&self.value); } } @@ -722,6 +746,50 @@ mod test { ); } + #[test] + fn commit_reports_a_publish_failure() { + #[derive(serde::Serialize, serde::Deserialize, Default, PartialEq, Debug)] + struct Doc { + a: u32, + } + + let track = moq_net::broadcast::Info::new() + .produce() + .create_track("test", None) + .unwrap(); + let mut producer = Producer::::new(track, ProducerConfig::default()); + + // A finished track can't take another group, so the publish behind the guard fails. + producer.finish().unwrap(); + + let mut guard = producer.lock(); + guard.a = 1; + assert!(matches!(guard.commit(), Err(crate::Error::Net(_)))); + } + + #[test] + fn commit_publishes_once() { + #[derive(serde::Serialize, serde::Deserialize, Default, PartialEq, Debug)] + struct Doc { + a: u32, + } + + let track = moq_net::broadcast::Info::new() + .produce() + .create_track("test", None) + .unwrap(); + let consumer = track.subscribe(None); + let mut producer = Producer::::new(track, cfg(0)); + + let mut guard = producer.lock(); + guard.a = 1; + guard.commit().unwrap(); + + // The drop that follows `commit` must not publish a second group. + producer.finish().unwrap(); + assert_eq!(consumer.latest(), Some(0)); + } + #[test] fn newer_group_supersedes_in_progress_reconstruction() { // A tight ratio fills group 0 with a couple of deltas, then forces a later update into a new diff --git a/rs/moq-json/src/stream.rs b/rs/moq-json/src/stream.rs index 712a2708e7..f2cd158ebc 100644 --- a/rs/moq-json/src/stream.rs +++ b/rs/moq-json/src/stream.rs @@ -11,8 +11,14 @@ //! catch-up machinery): the only reason to roll would be moq-net's per-group frame cap, which //! isn't worth working around here. A caller that wants to bound the record rate throttles at //! the source (e.g. the timeline's granularity); a consumer that finds a gap can fetch or -//! extrapolate. A late joiner reads whatever frames the relay still retains for the group; -//! deep history is served from a recording, not this live stream. +//! extrapolate. +//! +//! That single group is what bounds the log's history. moq-net caps a group's cached bytes, and a +//! consumer always starts at frame 0, so once the log outgrows that budget and the earliest frames +//! are evicted a new consumer fails with [`moq_net::Error::Lagged`] rather than reading a partial +//! log. (With compression the retained suffix would be undecodable anyway, since its DEFLATE window +//! depends on the evicted prefix.) The live stream is therefore bounded history by design; deep +//! history is served from a recording. use std::marker::PhantomData; use std::sync::{Arc, Mutex}; diff --git a/rs/moq-mux/src/catalog/hang/container.rs b/rs/moq-mux/src/catalog/hang/container.rs index cb30013b92..9a6ef1c0c0 100644 --- a/rs/moq-mux/src/catalog/hang/container.rs +++ b/rs/moq-mux/src/catalog/hang/container.rs @@ -27,10 +27,25 @@ impl TryFrom<&hang::catalog::Container> for Container { hang::catalog::Container::Legacy => Ok(Self::Legacy), hang::catalog::Container::Cmaf { init, .. } => Ok(Self::Cmaf(fmp4::Wire::from_init(init)?)), hang::catalog::Container::Loc => Ok(Self::Loc), + hang::catalog::Container::Unknown(unknown) => Err(crate::Error::unsupported_container(unknown)), } } } +/// Whether a rendition's frames can be parsed by this build, logging the ones dropped. +/// +/// The hang spec requires a consumer to ignore a rendition whose container `kind` it does not +/// recognize, so filter on this instead of failing the entire broadcast. +pub(crate) fn supported(rendition: &str, container: &hang::catalog::Container) -> bool { + match container { + hang::catalog::Container::Unknown(unknown) => { + tracing::warn!(rendition, kind = unknown.kind(), "ignoring unknown container"); + false + } + _ => true, + } +} + impl ContainerTrait for Container { type Error = crate::Error; diff --git a/rs/moq-mux/src/catalog/hang/mod.rs b/rs/moq-mux/src/catalog/hang/mod.rs index e7ea5d78ab..6621b929da 100644 --- a/rs/moq-mux/src/catalog/hang/mod.rs +++ b/rs/moq-mux/src/catalog/hang/mod.rs @@ -12,4 +12,5 @@ mod ext; pub use consumer::Consumer; pub use container::Container; +pub(crate) use container::supported; pub use ext::{Catalog, CatalogExt, Extra}; diff --git a/rs/moq-mux/src/catalog/producer.rs b/rs/moq-mux/src/catalog/producer.rs index e84629c24c..f2855f6730 100644 --- a/rs/moq-mux/src/catalog/producer.rs +++ b/rs/moq-mux/src/catalog/producer.rs @@ -137,6 +137,9 @@ impl Producer { } /// Get mutable access to the catalog, publishing it after any changes. + /// + /// The publish happens when the returned [`Guard`] drops and only warns on failure; call + /// [`Guard::commit`] instead to handle the error. pub fn lock(&mut self) -> Guard<'_, E> { Guard { catalog: self.current.lock().unwrap(), @@ -196,7 +199,9 @@ impl Producer { r.published = true; } let catalog = self.current.lock().unwrap().clone(); - emit(&mut self.hang, &mut self.hangz, &mut self.msf_track, &catalog); + if let Err(err) = emit(&mut self.hang, &mut self.hangz, &mut self.msf_track, &catalog) { + tracing::warn!(%err, "failed to publish the catalog"); + } } /// Build the media [`container::Producer`](crate::container::Producer) for `track`, recording its @@ -204,15 +209,15 @@ impl Producer { /// /// This is the 1:1 default. To share a timeline across aligned renditions, build the producer /// yourself and wire the shared timeline's recorder: - /// `container::Producer::new(track, container).with_recorder(catalog.timeline(shared).recorder())`, - /// and advertise `catalog.timeline(shared).section()` on each of their configs. + /// `container::Producer::new(track, container).with_recorder(catalog.timeline(shared)?.recorder())`, + /// and advertise `catalog.timeline(shared)?.section()` on each of their configs. pub fn media_producer( &self, track: moq_net::track::Producer, container: C, - ) -> crate::container::Producer { - let recorder = self.timeline(track.name()).recorder(); - crate::container::Producer::new(track, container).with_recorder(recorder) + ) -> crate::Result> { + let recorder = self.timeline(track.name())?.recorder(); + Ok(crate::container::Producer::new(track, container).with_recorder(recorder)) } /// The [`timeline::Producer`](crate::timeline::Producer) named `name`, creating its @@ -222,15 +227,18 @@ impl Producer { /// record group opens through its [`recorder`](crate::timeline::Producer::recorder). Two /// renditions naming the same timeline share it: an aligned transcode ladder records the source /// and has the rungs only advertise the same section. - pub fn timeline(&self, name: &str) -> crate::timeline::Producer { + /// + /// Errors on first use if the broadcast can't create the `.timeline.z` track, for example + /// because something else already took that name. + pub fn timeline(&self, name: &str) -> crate::Result { let mut timelines = self.timelines.lock().unwrap(); - timelines - .entry(name.to_string()) - .or_insert_with(|| { - crate::timeline::Producer::new(&mut self.broadcast.clone(), name) - .expect("failed to create timeline track") - }) - .clone() + if let Some(timeline) = timelines.get(name) { + return Ok(timeline.clone()); + } + + let timeline = crate::timeline::Producer::new(&mut self.broadcast.clone(), name)?; + timelines.insert(name.to_string(), timeline.clone()); + Ok(timeline) } /// Create a consumer for this catalog, receiving updates as they're published. @@ -255,7 +263,9 @@ impl Producer { /// Obtained via [`Producer::lock`]. Derefs to the [`Catalog`](super::hang::Catalog), so `video`/`audio` /// and (through the catalog's own deref) the extension sections are editable directly. /// -/// On drop, the hang, compressed-hang, and MSF catalog tracks are updated if the catalog was mutated. +/// On drop, the hang, compressed-hang, and MSF catalog tracks are updated if the catalog was +/// mutated. That publish can fail (a closed track, or an extension that won't serialize) and only +/// logs a warning; call [`commit`](Self::commit) instead to handle the error. pub struct Guard<'a, E: CatalogExt = ()> { catalog: MutexGuard<'a, Catalog>, hang: &'a mut moq_json::snapshot::Producer>, @@ -265,6 +275,38 @@ pub struct Guard<'a, E: CatalogExt = ()> { updated: bool, } +impl Guard<'_, E> { + /// Publish the edited catalog to every catalog track, returning any error. + /// + /// Consumes the guard, so the subsequent drop publishes nothing. A no-op if the catalog was never + /// mutated, and still withheld while a [`Reserved`](super::Reserved) gates the initial snapshot. + pub fn commit(mut self) -> crate::Result<()> { + self.publish() + } + + /// Publish a mutated catalog once, clearing the flag so it isn't published again. + fn publish(&mut self) -> crate::Result<()> { + if !self.updated { + return Ok(()); + } + self.updated = false; + + { + let mut r = self.reservations.lock().unwrap(); + // Withhold every emit while still buffering the initial reserved set; the mutation stays + // in `current` and `pending` marks it for the flush once the gate opens. + if !r.published && r.reservers != 0 { + r.pending = true; + return Ok(()); + } + r.pending = false; + r.published = true; + } + + emit(self.hang, self.hangz, self.msf_track, &self.catalog) + } +} + impl Deref for Guard<'_, E> { type Target = Catalog; @@ -304,23 +346,9 @@ impl Guard<'_, Extra> { impl Drop for Guard<'_, E> { fn drop(&mut self) { - if !self.updated { - return; + if let Err(err) = self.publish() { + tracing::warn!(%err, "failed to publish the catalog on guard drop"); } - - { - let mut r = self.reservations.lock().unwrap(); - // Withhold every emit while still buffering the initial reserved set; the mutation stays - // in `current` and `pending` marks it for the flush once the gate opens. - if !r.published && r.reservers != 0 { - r.pending = true; - return; - } - r.pending = false; - r.published = true; - } - - emit(self.hang, self.hangz, self.msf_track, &self.catalog); } } @@ -331,16 +359,19 @@ fn emit( hangz: &mut moq_json::snapshot::Producer>, msf_track: &mut moq_net::track::Producer, catalog: &Catalog, -) { +) -> crate::Result<()> { // One snapshot per group while deltas are disabled; the `.z` track carries the identical catalog. - let _ = hang.update(catalog); - let _ = hangz.update(catalog); + hang.update(catalog)?; + hangz.update(catalog)?; - let msf = to_msf(&catalog.media()); - if let Ok(mut group) = msf_track.append_group() { - let _ = group.write_frame(moq_net::Timestamp::now(), msf.to_json().expect("invalid MSF catalog")); - let _ = group.finish(); - } + // The MSF catalog is derived from our own types, so a serialize failure means an extension broke + // the shape; report it like any other JSON failure rather than panicking. + let msf = to_msf(&catalog.media()).to_json().map_err(moq_json::Error::from)?; + let mut group = msf_track.append_group()?; + group.write_frame(moq_net::Timestamp::now(), msf)?; + group.finish()?; + + Ok(()) } /// Determine the SAP starting type for a given video codec. @@ -476,6 +507,51 @@ mod test { assert_eq!(got_compressed, expected); } + #[test] + fn commit_reports_a_publish_failure() { + let mut broadcast = moq_net::broadcast::Info::new().produce(); + let mut catalog = Producer::new(&mut broadcast).unwrap(); + + // Finished tracks can't take another group, so the publish behind the guard fails. + // The producer is a shared handle, so finish a clone and keep this one to publish through. + catalog.clone().finish().unwrap(); + + let mut guard = catalog.lock(); + guard + .audio + .renditions + .insert("audio0".to_string(), AudioConfig::new(AudioCodec::Opus, 48_000, 2)); + assert!(guard.commit().is_err()); + } + + #[test] + fn commit_publishes_once() { + let mut broadcast = moq_net::broadcast::Info::new().produce(); + let mut catalog = Producer::new(&mut broadcast).unwrap(); + let track = catalog.hang.consume(); + + let mut guard = catalog.lock(); + guard + .audio + .renditions + .insert("audio0".to_string(), AudioConfig::new(AudioCodec::Opus, 48_000, 2)); + guard.commit().unwrap(); + + // The drop that follows `commit` must not publish a second snapshot. + catalog.finish().unwrap(); + assert_eq!(track.latest(), Some(0)); + } + + #[test] + fn timeline_reports_a_track_collision() { + let mut broadcast = moq_net::broadcast::Info::new().produce(); + let catalog = Producer::new(&mut broadcast).unwrap(); + + // Something else already took the name the timeline track wants. + let _taken = broadcast.create_track("video0.timeline.z", None).unwrap(); + assert!(catalog.timeline("video0").is_err()); + } + fn h264_config() -> VideoConfig { let mut config = VideoConfig::new(H264 { profile: 0x64, diff --git a/rs/moq-mux/src/catalog/tracks.rs b/rs/moq-mux/src/catalog/tracks.rs index 28850e7167..9495395e04 100644 --- a/rs/moq-mux/src/catalog/tracks.rs +++ b/rs/moq-mux/src/catalog/tracks.rs @@ -533,7 +533,7 @@ mod tests { let catalog = super::super::Producer::new(&mut broadcast).unwrap(); let reserved = catalog.reserve(); - let shared = catalog.timeline("video"); + let shared = catalog.timeline("video").unwrap(); let mut source = reserved.video("video0"); let mut rung = reserved.video("video1"); drop(reserved); @@ -637,7 +637,7 @@ mod tests { // The caller advertises the timeline explicitly, exactly as an importer does for video/audio. let mut config = telemetry(None); - config.timeline = Some(catalog.timeline("gps").section()); + config.timeline = Some(catalog.timeline("gps").unwrap().section()); rendition.set(config); feed(&mut rendition); diff --git a/rs/moq-mux/src/codec/aac/import.rs b/rs/moq-mux/src/codec/aac/import.rs index 0aa7e601e4..d87afbdfb4 100644 --- a/rs/moq-mux/src/codec/aac/import.rs +++ b/rs/moq-mux/src/codec/aac/import.rs @@ -23,18 +23,18 @@ impl Import { track: moq_net::track::Producer, reserved: crate::catalog::Reserved, mut config: hang::catalog::AudioConfig, - ) -> Self { + ) -> crate::Result { tracing::debug!(name = ?track.name(), ?config, "starting track"); // Advertise this rendition's timeline before publishing (the generic set() no longer does). - config.timeline = Some(reserved.producer().timeline(track.name()).section()); + config.timeline = Some(reserved.producer().timeline(track.name())?.section()); let mut rendition = reserved.audio(track.name()); rendition.set(config); - Self { + Ok(Self { track: reserved .producer() - .media_producer(track, crate::catalog::hang::Container::Legacy), + .media_producer(track, crate::catalog::hang::Container::Legacy)?, rendition, - } + }) } /// The MoQ track name this importer publishes on. @@ -63,8 +63,8 @@ impl Import { } /// Abort the track with `err` instead of finishing it cleanly, so subscribers - /// see the real cause rather than [`moq_net::Error::Dropped`]. - pub fn abort(&mut self, err: moq_net::Error) { + /// see the real cause rather than [`moq_net::Error::Dropped`]. Consumes this importer. + pub fn abort(self, err: moq_net::Error) { self.track.abort(err); } diff --git a/rs/moq-mux/src/codec/av1/import.rs b/rs/moq-mux/src/codec/av1/import.rs index cc6905c923..662e3995b9 100644 --- a/rs/moq-mux/src/codec/av1/import.rs +++ b/rs/moq-mux/src/codec/av1/import.rs @@ -43,13 +43,13 @@ impl Import { track: moq_net::track::Producer, reserved: crate::catalog::Reserved, hint: crate::catalog::VideoHint, - ) -> Self { + ) -> crate::Result { let rendition = reserved.video(track.name()); - let catalog = crate::codec::video::Catalog::new(&reserved, track.name(), hint); + let catalog = crate::codec::video::Catalog::new(&reserved, track.name(), hint)?; let mut import = Self { track: reserved .producer() - .media_producer(track, crate::catalog::hang::Container::Legacy), + .media_producer(track, crate::catalog::hang::Container::Legacy)?, rendition, catalog, last_seq: None, @@ -57,7 +57,7 @@ impl Import { if let Some(config) = import.catalog.initial_config() { import.apply_config(config); } - import + Ok(import) } /// Resolve the codec config from a sequence header / av1C and other metadata. @@ -212,8 +212,8 @@ impl Import { } /// Abort the track with `err` instead of finishing it cleanly, so subscribers - /// see the real cause rather than [`moq_net::Error::Dropped`]. - pub fn abort(&mut self, err: moq_net::Error) { + /// see the real cause rather than [`moq_net::Error::Dropped`]. Consumes this importer. + pub fn abort(self, err: moq_net::Error) { self.track.abort(err); } diff --git a/rs/moq-mux/src/codec/flac/import.rs b/rs/moq-mux/src/codec/flac/import.rs index 369a2aefce..4734612bf8 100644 --- a/rs/moq-mux/src/codec/flac/import.rs +++ b/rs/moq-mux/src/codec/flac/import.rs @@ -26,18 +26,18 @@ impl Import { track: moq_net::track::Producer, reserved: crate::catalog::Reserved, mut config: hang::catalog::AudioConfig, - ) -> Self { + ) -> crate::Result { tracing::debug!(name = ?track.name(), ?config, "starting track"); // Advertise this rendition's timeline before publishing (the generic set() no longer does). - config.timeline = Some(reserved.producer().timeline(track.name()).section()); + config.timeline = Some(reserved.producer().timeline(track.name())?.section()); let mut rendition = reserved.audio(track.name()); rendition.set(config); - Self { + Ok(Self { track: reserved .producer() - .media_producer(track, crate::catalog::hang::Container::Legacy), + .media_producer(track, crate::catalog::hang::Container::Legacy)?, rendition, - } + }) } /// A watch-only handle to this track's subscriber demand. @@ -53,8 +53,8 @@ impl Import { } /// Abort the track with `err` instead of finishing it cleanly, so subscribers - /// see the real cause rather than [`moq_net::Error::Dropped`]. - pub fn abort(&mut self, err: moq_net::Error) { + /// see the real cause rather than [`moq_net::Error::Dropped`]. Consumes this importer. + pub fn abort(self, err: moq_net::Error) { self.track.abort(err); } diff --git a/rs/moq-mux/src/codec/h264/import.rs b/rs/moq-mux/src/codec/h264/import.rs index ae4fd887d5..570c00ee29 100644 --- a/rs/moq-mux/src/codec/h264/import.rs +++ b/rs/moq-mux/src/codec/h264/import.rs @@ -46,14 +46,14 @@ impl Import { track: moq_net::track::Producer, reserved: crate::catalog::Reserved, hint: crate::catalog::VideoHint, - ) -> Self { + ) -> crate::Result { let rendition = reserved.video(track.name()); - let catalog = crate::codec::video::Catalog::new(&reserved, track.name(), hint); + let catalog = crate::codec::video::Catalog::new(&reserved, track.name(), hint)?; let mut import = Self { avc1: false, track: reserved .producer() - .media_producer(track, crate::catalog::hang::Container::Legacy), + .media_producer(track, crate::catalog::hang::Container::Legacy)?, rendition, catalog, last_sps: None, @@ -61,7 +61,7 @@ impl Import { if let Some(config) = import.catalog.initial_config() { import.apply_config(config); } - import + Ok(import) } /// Resolve the codec config from the codec's leading bytes. @@ -140,8 +140,8 @@ impl Import { } /// Abort the track with `err` instead of finishing it cleanly, so subscribers - /// see the real cause rather than [`moq_net::Error::Dropped`]. - pub fn abort(&mut self, err: moq_net::Error) { + /// see the real cause rather than [`moq_net::Error::Dropped`]. Consumes this importer. + pub fn abort(self, err: moq_net::Error) { self.track.abort(err); } @@ -289,7 +289,7 @@ mod tests { avcc.extend_from_slice(&[0x01, 0x00, 0x04, 0x68, 0xce, 0x3c, 0x80]); // num_pps + pps let (track, catalog) = setup("video"); - let mut import = Import::new(track, catalog.reserve(), Default::default()); + let mut import = Import::new(track, catalog.reserve(), Default::default()).unwrap(); // initialize() must not consume the buffer (the split owns the consume). let buf = bytes::BytesMut::from(avcc.as_slice()); import.initialize(&buf).expect("initialize avc1"); @@ -325,7 +325,7 @@ mod tests { let mut split = Split::new(); let (track, catalog) = setup("video"); - let mut import = Import::new(track, catalog.reserve(), Default::default()); + let mut import = Import::new(track, catalog.reserve(), Default::default()).unwrap(); assert!( catalog.snapshot().video.renditions.is_empty(), "no config before any frame" @@ -369,7 +369,7 @@ mod tests { let mut split = Split::new(); let (track, catalog) = setup("video"); - let mut import = Import::new(track, catalog.reserve(), Default::default()); + let mut import = Import::new(track, catalog.reserve(), Default::default()).unwrap(); let pts = moq_net::Timestamp::from_micros(0).unwrap(); let mut frames = split.decode(&annexb, pts).expect("split open-GOP AU"); @@ -401,7 +401,7 @@ mod tests { let mut split = Split::new(); let (track, catalog) = setup("video"); - let mut import = Import::new(track, catalog.reserve(), Default::default()); + let mut import = Import::new(track, catalog.reserve(), Default::default()).unwrap(); let pts = moq_net::Timestamp::from_micros(0).unwrap(); let mut frames = split.decode(&annexb, pts).expect("split keyframe"); @@ -424,7 +424,7 @@ mod tests { let mut split = Split::new(); let (track, catalog) = setup("video"); - let mut import = Import::new(track, catalog.reserve(), Default::default()); + let mut import = Import::new(track, catalog.reserve(), Default::default()).unwrap(); let pts = moq_net::Timestamp::from_micros(0).unwrap(); let mut frames = split.decode(&annexb, pts).expect("split delta"); diff --git a/rs/moq-mux/src/codec/h265/import.rs b/rs/moq-mux/src/codec/h265/import.rs index ba9360adf2..b9b0ad3ba2 100644 --- a/rs/moq-mux/src/codec/h265/import.rs +++ b/rs/moq-mux/src/codec/h265/import.rs @@ -45,13 +45,13 @@ impl Import { track: moq_net::track::Producer, reserved: crate::catalog::Reserved, hint: crate::catalog::VideoHint, - ) -> Self { + ) -> crate::Result { let rendition = reserved.video(track.name()); - let catalog = crate::codec::video::Catalog::new(&reserved, track.name(), hint); + let catalog = crate::codec::video::Catalog::new(&reserved, track.name(), hint)?; let mut import = Self { track: reserved .producer() - .media_producer(track, crate::catalog::hang::Container::Legacy), + .media_producer(track, crate::catalog::hang::Container::Legacy)?, rendition, catalog, last_sps: None, @@ -59,7 +59,7 @@ impl Import { if let Some(config) = import.catalog.initial_config() { import.apply_config(config); } - import + Ok(import) } /// Resolve the codec config from VPS/SPS/PPS and other non-slice NALs. @@ -102,8 +102,8 @@ impl Import { } /// Abort the track with `err` instead of finishing it cleanly, so subscribers - /// see the real cause rather than [`moq_net::Error::Dropped`]. - pub fn abort(&mut self, err: moq_net::Error) { + /// see the real cause rather than [`moq_net::Error::Dropped`]. Consumes this importer. + pub fn abort(self, err: moq_net::Error) { self.track.abort(err); } diff --git a/rs/moq-mux/src/codec/legacy.rs b/rs/moq-mux/src/codec/legacy.rs index 91337b4036..0e1fb82a4a 100644 --- a/rs/moq-mux/src/codec/legacy.rs +++ b/rs/moq-mux/src/codec/legacy.rs @@ -127,7 +127,7 @@ impl Import { track: moq_net::track::Producer, reserved: crate::catalog::Reserved, config: Config, - ) -> Self { + ) -> crate::Result { let mut audio_config = hang::catalog::AudioConfig::new(descriptor.codec.clone(), config.sample_rate, config.channel_count); audio_config.container = hang::catalog::Container::Legacy; @@ -138,16 +138,16 @@ impl Import { tracing::debug!(name = ?track.name(), config = ?audio_config, "starting track"); // Advertise this rendition's timeline before publishing (the generic set() no longer does). - audio_config.timeline = Some(reserved.producer().timeline(track.name()).section()); + audio_config.timeline = Some(reserved.producer().timeline(track.name())?.section()); let mut rendition = reserved.audio(track.name()); rendition.set(audio_config); - Self { + Ok(Self { track: reserved .producer() - .media_producer(track, crate::catalog::hang::Container::Legacy), + .media_producer(track, crate::catalog::hang::Container::Legacy)?, rendition, - } + }) } /// The MoQ track name. @@ -163,8 +163,8 @@ impl Import { } /// Abort the track with `err` instead of finishing it cleanly, so subscribers - /// see the real cause rather than [`moq_net::Error::Dropped`]. - pub fn abort(&mut self, err: moq_net::Error) { + /// see the real cause rather than [`moq_net::Error::Dropped`]. Consumes this importer. + pub fn abort(self, err: moq_net::Error) { self.track.abort(err); } diff --git a/rs/moq-mux/src/codec/mp3.rs b/rs/moq-mux/src/codec/mp3.rs index 361c483688..6c081c9c78 100644 --- a/rs/moq-mux/src/codec/mp3.rs +++ b/rs/moq-mux/src/codec/mp3.rs @@ -114,18 +114,18 @@ impl Import { track: moq_net::track::Producer, reserved: crate::catalog::Reserved, mut config: hang::catalog::AudioConfig, - ) -> Self { + ) -> crate::Result { tracing::debug!(name = ?track.name(), ?config, "starting track"); // Advertise this rendition's timeline before publishing (the generic set() no longer does). - config.timeline = Some(reserved.producer().timeline(track.name()).section()); + config.timeline = Some(reserved.producer().timeline(track.name())?.section()); let mut rendition = reserved.audio(track.name()); rendition.set(config); - Self { + Ok(Self { track: reserved .producer() - .media_producer(track, crate::catalog::hang::Container::Legacy), + .media_producer(track, crate::catalog::hang::Container::Legacy)?, rendition, - } + }) } /// A watch-only handle to this track's subscriber demand. @@ -141,8 +141,8 @@ impl Import { } /// Abort the track with `err` instead of finishing it cleanly, so subscribers - /// see the real cause rather than [`moq_net::Error::Dropped`]. - pub fn abort(&mut self, err: moq_net::Error) { + /// see the real cause rather than [`moq_net::Error::Dropped`]. Consumes this importer. + pub fn abort(self, err: moq_net::Error) { self.track.abort(err); } diff --git a/rs/moq-mux/src/codec/opus/import.rs b/rs/moq-mux/src/codec/opus/import.rs index 4586afa2d3..f79279752d 100644 --- a/rs/moq-mux/src/codec/opus/import.rs +++ b/rs/moq-mux/src/codec/opus/import.rs @@ -26,18 +26,18 @@ impl Import { track: moq_net::track::Producer, reserved: crate::catalog::Reserved, mut config: hang::catalog::AudioConfig, - ) -> Self { + ) -> crate::Result { tracing::debug!(name = ?track.name(), ?config, "starting track"); // Advertise this rendition's timeline before publishing (the generic set() no longer does). - config.timeline = Some(reserved.producer().timeline(track.name()).section()); + config.timeline = Some(reserved.producer().timeline(track.name())?.section()); let mut rendition = reserved.audio(track.name()); rendition.set(config); - Self { + Ok(Self { track: reserved .producer() - .media_producer(track, crate::catalog::hang::Container::Legacy), + .media_producer(track, crate::catalog::hang::Container::Legacy)?, rendition, - } + }) } /// The MoQ track name this importer publishes on. @@ -58,8 +58,8 @@ impl Import { } /// Abort the track with `err` instead of finishing it cleanly, so subscribers - /// see the real cause rather than [`moq_net::Error::Dropped`]. - pub fn abort(&mut self, err: moq_net::Error) { + /// see the real cause rather than [`moq_net::Error::Dropped`]. Consumes this importer. + pub fn abort(self, err: moq_net::Error) { self.track.abort(err); } diff --git a/rs/moq-mux/src/codec/video.rs b/rs/moq-mux/src/codec/video.rs index 22c226540c..23af13c70d 100644 --- a/rs/moq-mux/src/codec/video.rs +++ b/rs/moq-mux/src/codec/video.rs @@ -26,12 +26,12 @@ pub(crate) struct Catalog { impl Catalog { /// Snapshot the timeline for the rendition named `name`, and hold `hint` for every publish. - pub(crate) fn new(reserved: &Reserved, name: &str, hint: VideoHint) -> Self { - Self { - timeline: reserved.producer().timeline(name).section(), + pub(crate) fn new(reserved: &Reserved, name: &str, hint: VideoHint) -> crate::Result { + Ok(Self { + timeline: reserved.producer().timeline(name)?.section(), hint, last: None, - } + }) } /// The config the hint alone resolves to, for importers that publish the catalog before parsing diff --git a/rs/moq-mux/src/codec/vp8/import.rs b/rs/moq-mux/src/codec/vp8/import.rs index 98e0823cf0..959ba34694 100644 --- a/rs/moq-mux/src/codec/vp8/import.rs +++ b/rs/moq-mux/src/codec/vp8/import.rs @@ -30,20 +30,20 @@ impl Import { track: moq_net::track::Producer, reserved: crate::catalog::Reserved, hint: crate::catalog::VideoHint, - ) -> Self { + ) -> crate::Result { let rendition = reserved.video(track.name()); - let catalog = crate::codec::video::Catalog::new(&reserved, track.name(), hint); + let catalog = crate::codec::video::Catalog::new(&reserved, track.name(), hint)?; let mut import = Self { track: reserved .producer() - .media_producer(track, crate::catalog::hang::Container::Legacy), + .media_producer(track, crate::catalog::hang::Container::Legacy)?, rendition, catalog, }; if let Some(config) = import.catalog.initial_config() { import.apply_config(config); } - import + Ok(import) } /// Initialize the importer. @@ -119,8 +119,8 @@ impl Import { } /// Abort the track with `err` instead of finishing it cleanly, so subscribers - /// see the real cause rather than [`moq_net::Error::Dropped`]. - pub fn abort(&mut self, err: moq_net::Error) { + /// see the real cause rather than [`moq_net::Error::Dropped`]. Consumes this importer. + pub fn abort(self, err: moq_net::Error) { self.track.abort(err); } @@ -157,7 +157,7 @@ mod tests { #[tokio::test(start_paused = true)] async fn imports_keyframe_then_interframe() { let (track, catalog) = setup(); - let mut import = super::Import::new(track, catalog.reserve(), Default::default()); + let mut import = super::Import::new(track, catalog.reserve(), Default::default()).unwrap(); // Empty init buffer: the catalog is filled on the first key frame. import.initialize(&[]).unwrap(); @@ -188,7 +188,7 @@ mod tests { #[tokio::test(start_paused = true)] async fn rejects_interframe_first() { let (track, catalog) = setup(); - let mut import = super::Import::new(track, catalog.reserve(), Default::default()); + let mut import = super::Import::new(track, catalog.reserve(), Default::default()).unwrap(); let interframe = Bytes::from_static(&[0x31, 0x00, 0x00, 0xaa, 0xbb]); assert!( diff --git a/rs/moq-mux/src/codec/vp9/import.rs b/rs/moq-mux/src/codec/vp9/import.rs index fd4fd74576..a9a613f9e7 100644 --- a/rs/moq-mux/src/codec/vp9/import.rs +++ b/rs/moq-mux/src/codec/vp9/import.rs @@ -30,20 +30,20 @@ impl Import { track: moq_net::track::Producer, reserved: crate::catalog::Reserved, hint: crate::catalog::VideoHint, - ) -> Self { + ) -> crate::Result { let rendition = reserved.video(track.name()); - let catalog = crate::codec::video::Catalog::new(&reserved, track.name(), hint); + let catalog = crate::codec::video::Catalog::new(&reserved, track.name(), hint)?; let mut import = Self { track: reserved .producer() - .media_producer(track, crate::catalog::hang::Container::Legacy), + .media_producer(track, crate::catalog::hang::Container::Legacy)?, rendition, catalog, }; if let Some(config) = import.catalog.initial_config() { import.apply_config(config); } - import + Ok(import) } /// Initialize the importer. @@ -119,8 +119,8 @@ impl Import { } /// Abort the track with `err` instead of finishing it cleanly, so subscribers - /// see the real cause rather than [`moq_net::Error::Dropped`]. - pub fn abort(&mut self, err: moq_net::Error) { + /// see the real cause rather than [`moq_net::Error::Dropped`]. Consumes this importer. + pub fn abort(self, err: moq_net::Error) { self.track.abort(err); } @@ -158,7 +158,7 @@ mod tests { #[tokio::test(start_paused = true)] async fn imports_keyframe_then_interframe() { let (track, catalog) = setup(); - let mut import = super::Import::new(track, catalog.reserve(), Default::default()); + let mut import = super::Import::new(track, catalog.reserve(), Default::default()).unwrap(); import.initialize(&[]).unwrap(); assert!(catalog.snapshot().video.renditions.is_empty()); @@ -184,7 +184,7 @@ mod tests { #[tokio::test(start_paused = true)] async fn rejects_interframe_first() { let (track, catalog) = setup(); - let mut import = super::Import::new(track, catalog.reserve(), Default::default()); + let mut import = super::Import::new(track, catalog.reserve(), Default::default()).unwrap(); let interframe = Bytes::from_static(&[0x84, 0x00, 0x00]); assert!( diff --git a/rs/moq-mux/src/container/consumer.rs b/rs/moq-mux/src/container/consumer.rs index 395d93a5b0..11ba6c27d2 100644 --- a/rs/moq-mux/src/container/consumer.rs +++ b/rs/moq-mux/src/container/consumer.rs @@ -1873,7 +1873,7 @@ mod tests { let consumer_track = track.subscribe(None); let mut consumer = Consumer::new(consumer_track, Container::Legacy).with_latency(Duration::from_millis(500)); - let mut group0 = track.create_group(moq_net::group::Info { sequence: 0 }).unwrap(); + let group0 = track.create_group(moq_net::group::Info { sequence: 0 }).unwrap(); group0.abort(moq_net::Error::Cancel).unwrap(); write_group(&mut track, 1, &[ts(30_000)]); diff --git a/rs/moq-mux/src/container/flv/export.rs b/rs/moq-mux/src/container/flv/export.rs index e16e72fcc3..eb0925d714 100644 --- a/rs/moq-mux/src/container/flv/export.rs +++ b/rs/moq-mux/src/container/flv/export.rs @@ -643,6 +643,10 @@ fn ensure_legacy(container: &Container, kind: &str, name: &str) -> anyhow::Resul match container { Container::Legacy | Container::Loc => Ok(()), Container::Cmaf { .. } => anyhow::bail!("FLV export does not support CMAF {kind} track '{name}'"), + Container::Unknown(unknown) => anyhow::bail!( + "FLV export does not support container '{}' on {kind} track '{name}'", + unknown.kind().unwrap_or("") + ), } } diff --git a/rs/moq-mux/src/container/flv/import.rs b/rs/moq-mux/src/container/flv/import.rs index fbe81bd1e8..e89111c733 100644 --- a/rs/moq-mux/src/container/flv/import.rs +++ b/rs/moq-mux/src/container/flv/import.rs @@ -503,7 +503,7 @@ impl Import { // site (the producer reports MissingKeyframe), so a mid-GOP join works. track: self .catalog - .media_producer(net_track, crate::catalog::hang::Container::Legacy), + .media_producer(net_track, crate::catalog::hang::Container::Legacy)?, config, }, ); @@ -527,7 +527,7 @@ impl Import { AudioStream { track: self .catalog - .media_producer(net_track, crate::catalog::hang::Container::Legacy), + .media_producer(net_track, crate::catalog::hang::Container::Legacy)?, config, }, ); @@ -579,18 +579,19 @@ impl Import { /// Abort every track with `err`, so consumers see the real cause instead of the /// generic [`moq_net::Error::Dropped`] a bare drop surfaces. The counterpart to /// [`Self::finish`] for a failed teardown (e.g. the RTMP client disconnected). - pub fn abort(&mut self, err: moq_net::Error) { - for stream in self.video.values_mut() { + /// Consumes the importer. + pub fn abort(mut self, err: moq_net::Error) { + self.unregister(); + for stream in std::mem::take(&mut self.video).into_values() { stream.track.abort(err.clone()); } - for stream in self.audio.values_mut() { + for stream in std::mem::take(&mut self.audio).into_values() { stream.track.abort(err.clone()); } } -} -impl Drop for Import { - fn drop(&mut self) { + /// Drop every rendition this importer registered from the catalog. + fn unregister(&mut self) { let mut catalog = self.catalog.lock(); for stream in self.video.values() { catalog.video.renditions.remove(stream.track.name()); @@ -601,6 +602,12 @@ impl Drop for Import { } } +impl Drop for Import { + fn drop(&mut self) { + self.unregister(); + } +} + /// The multitrack framing common to every track in one tag: the layout and the /// real `VideoPacketType`/`AudioPacketType`, plus the shared FourCC (present for /// every layout except `ManyTracksManyCodecs`, where each track carries its own). diff --git a/rs/moq-mux/src/container/flv/import_test.rs b/rs/moq-mux/src/container/flv/import_test.rs index 91f230b23d..780046cc77 100644 --- a/rs/moq-mux/src/container/flv/import_test.rs +++ b/rs/moq-mux/src/container/flv/import_test.rs @@ -133,8 +133,6 @@ async fn import_emits_frames() { assert!(frame.keyframe); // The payload is the length-prefixed NALU, carried through verbatim. assert_eq!(frame.payload.as_ref(), &[0, 0, 0, 5, 0x65, 0x88, 0x84, 0x21, 0x00]); - - drop(importer); } /// Bytes split across two `decode` calls still reassemble into whole tags. diff --git a/rs/moq-mux/src/container/fmp4/export.rs b/rs/moq-mux/src/container/fmp4/export.rs index e309195387..3d6e19fd1f 100644 --- a/rs/moq-mux/src/container/fmp4/export.rs +++ b/rs/moq-mux/src/container/fmp4/export.rs @@ -318,6 +318,18 @@ impl Export { } fn update_catalog(&mut self, catalog: &Catalog) -> Result<()> { + // A rendition we can't parse is ignored rather than failing the whole export. + let mut catalog = catalog.clone(); + catalog + .video + .renditions + .retain(|name, config| crate::catalog::hang::supported(name, &config.container)); + catalog + .audio + .renditions + .retain(|name, config| crate::catalog::hang::supported(name, &config.container)); + let catalog = &catalog; + let mut active: HashMap = HashMap::new(); for name in catalog.video.renditions.keys() { active.insert(name.clone(), ()); @@ -335,7 +347,7 @@ impl Export { continue; } let source = ExportSource::for_video(&self.source, name, config, self.latency)?; - let timescale = catalog_timescale_video(config); + let timescale = catalog_timescale_video(config)?; // A zero / NaN / infinite framerate would make `1.0 / fps` non-finite and panic // `Duration::from_secs_f64`; fall back to the default in that case. let framerate = config @@ -366,7 +378,7 @@ impl Export { continue; } let source = ExportSource::for_audio(&self.source, name, config, self.latency)?; - let timescale = catalog_timescale_audio(config); + let timescale = catalog_timescale_audio(config)?; self.tracks.insert( name.clone(), Fmp4Track { @@ -435,6 +447,7 @@ impl Export { }); traks.push(trak); } + Container::Unknown(unknown) => return Err(crate::Error::unsupported_container(unknown)), } } @@ -456,6 +469,7 @@ impl Export { }); traks.push(trak); } + Container::Unknown(unknown) => return Err(crate::Error::unsupported_container(unknown)), } } @@ -679,20 +693,22 @@ fn next_timestamp(frames: &[Frame], successor: Option<&Frame>, index: usize) -> .or_else(|| successor.map(|next| next.timestamp)) } -pub(crate) fn catalog_timescale_video(config: &VideoConfig) -> u64 { - match &config.container { +pub(crate) fn catalog_timescale_video(config: &VideoConfig) -> Result { + Ok(match &config.container { Container::Cmaf { init, .. } => { parse_timescale_from_init(init).unwrap_or_else(|_| crate::container::fmp4::default_video_timescale(config)) } Container::Loc | Container::Legacy => crate::container::fmp4::default_video_timescale(config), - } + Container::Unknown(unknown) => return Err(crate::Error::unsupported_container(unknown)), + }) } -pub(crate) fn catalog_timescale_audio(config: &hang::catalog::AudioConfig) -> u64 { - match &config.container { +pub(crate) fn catalog_timescale_audio(config: &hang::catalog::AudioConfig) -> Result { + Ok(match &config.container { Container::Cmaf { init, .. } => parse_timescale_from_init(init).unwrap_or(config.sample_rate as u64), Container::Loc | Container::Legacy => config.sample_rate as u64, - } + Container::Unknown(unknown) => return Err(crate::Error::unsupported_container(unknown)), + }) } fn parse_timescale_from_init(init: &[u8]) -> Result { diff --git a/rs/moq-mux/src/container/fmp4/import.rs b/rs/moq-mux/src/container/fmp4/import.rs index a163fae63f..bec209e3ae 100644 --- a/rs/moq-mux/src/container/fmp4/import.rs +++ b/rs/moq-mux/src/container/fmp4/import.rs @@ -238,7 +238,7 @@ impl Import { // Each track indexes its own group opens: audio and video group boundaries differ, so a // per-track timeline (the 1:1 default) is correct here, not a shared one. - let timeline = self.catalog.timeline(track.name()); + let timeline = self.catalog.timeline(track.name())?; let detect_bitrate = match kind { TrackKind::Video => { @@ -857,16 +857,32 @@ impl Import { } /// Abort all tracks with `err` instead of finishing, so subscribers see the real - /// cause rather than [`moq_net::Error::Dropped`]. - pub fn abort(&mut self, err: moq_net::Error) { - for track in self.tracks.values_mut() { - if let Some(mut g) = track.group.take() { + /// cause rather than [`moq_net::Error::Dropped`]. Consumes the importer. + pub fn abort(mut self, err: moq_net::Error) { + self.unregister(); + for mut track in std::mem::take(&mut self.tracks).into_values() { + if let Some(g) = track.group.take() { let _ = g.abort(err.clone()); } let _ = track.track.abort(err.clone()); } } + /// Drop every rendition this importer registered from the catalog. + fn unregister(&mut self) { + let mut catalog = self.catalog.lock(); + for track in self.tracks.values() { + match track.kind { + TrackKind::Video => { + catalog.video.renditions.remove(track.track.name()); + } + TrackKind::Audio => { + catalog.audio.renditions.remove(track.track.name()); + } + } + } + } + /// Close the current group on every track and open the next one at `sequence`. /// /// Broadcast-wide: every track inside this fMP4 import advances together; per-track @@ -920,17 +936,6 @@ fn set_detected_bitrate( impl Drop for Import { fn drop(&mut self) { - let mut catalog = self.catalog.lock(); - - for track in self.tracks.values() { - match track.kind { - TrackKind::Video => { - catalog.video.renditions.remove(track.track.name()); - } - TrackKind::Audio => { - catalog.audio.renditions.remove(track.track.name()); - } - } - } + self.unregister(); } } diff --git a/rs/moq-mux/src/container/fmp4/muxer.rs b/rs/moq-mux/src/container/fmp4/muxer.rs index fe4b7b98cd..d0ee633826 100644 --- a/rs/moq-mux/src/container/fmp4/muxer.rs +++ b/rs/moq-mux/src/container/fmp4/muxer.rs @@ -66,7 +66,7 @@ impl Muxer { container, transform: build_video_transform(config), description: config.description.as_ref().filter(|b| !b.is_empty()).cloned(), - timescale: catalog_timescale_video(config), + timescale: catalog_timescale_video(config)?, default_frame: Duration::from_secs_f64(1.0 / framerate), kind: Kind::Video(config.clone()), }) @@ -79,7 +79,7 @@ impl Muxer { container, transform: None, description: config.description.as_ref().filter(|b| !b.is_empty()).cloned(), - timescale: catalog_timescale_audio(config), + timescale: catalog_timescale_audio(config)?, // Fallback for a duration-less trailing sample (~1024 samples per frame). default_frame: Duration::from_secs_f64(1024.0 / config.sample_rate.max(1) as f64), kind: Kind::Audio(config.clone()), @@ -162,6 +162,7 @@ impl Muxer { }); traks.push(trak); } + CatalogContainer::Unknown(unknown) => return Err(crate::Error::unsupported_container(unknown)), } let ftyp = ftyp.unwrap_or(mp4_atom::Ftyp { diff --git a/rs/moq-mux/src/container/mkv/export.rs b/rs/moq-mux/src/container/mkv/export.rs index 7e06d8bfcc..5125f79689 100644 --- a/rs/moq-mux/src/container/mkv/export.rs +++ b/rs/moq-mux/src/container/mkv/export.rs @@ -531,6 +531,7 @@ fn ensure_legacy(container: &Container, kind: &str, name: &str) -> Result<()> { name: name.to_string(), } .into()), + Container::Unknown(unknown) => Err(crate::Error::unsupported_container(unknown)), } } diff --git a/rs/moq-mux/src/container/mkv/import.rs b/rs/moq-mux/src/container/mkv/import.rs index 00aa83450e..b3c820c15c 100644 --- a/rs/moq-mux/src/container/mkv/import.rs +++ b/rs/moq-mux/src/container/mkv/import.rs @@ -295,7 +295,7 @@ impl Import { kind, track: self .catalog - .media_producer(track, crate::catalog::hang::Container::Legacy), + .media_producer(track, crate::catalog::hang::Container::Legacy)?, group: None, last_emitted_ticks: None, }, @@ -407,19 +407,19 @@ impl Import { } /// Abort all tracks with `err` instead of finishing, so subscribers see the real - /// cause rather than [`moq_net::Error::Dropped`]. - pub fn abort(&mut self, err: moq_net::Error) { - for track in self.tracks.values_mut() { - if let Some(mut g) = track.group.take() { + /// cause rather than [`moq_net::Error::Dropped`]. Consumes the importer. + pub fn abort(mut self, err: moq_net::Error) { + self.unregister(); + for mut track in std::mem::take(&mut self.tracks).into_values() { + if let Some(g) = track.group.take() { let _ = g.abort(err.clone()); } track.track.abort(err.clone()); } } -} -impl Drop for Import { - fn drop(&mut self) { + /// Drop every rendition this importer registered from the catalog. + fn unregister(&mut self) { let mut catalog = self.catalog.lock(); for track in self.tracks.values() { match track.kind { @@ -434,6 +434,12 @@ impl Drop for Import { } } +impl Drop for Import { + fn drop(&mut self) { + self.unregister(); + } +} + fn build_video_config( codec_id: &str, codec_private: Option<&Bytes>, diff --git a/rs/moq-mux/src/container/producer.rs b/rs/moq-mux/src/container/producer.rs index 5fa8cec656..1db0163532 100644 --- a/rs/moq-mux/src/container/producer.rs +++ b/rs/moq-mux/src/container/producer.rs @@ -233,10 +233,10 @@ impl Producer { /// The counterpart to [`Self::finish`] for a failed teardown: consumers observe /// `err` instead of the generic [`moq_net::Error::Dropped`] a bare drop surfaces, /// so the real cause (a disconnect, a decode failure) reaches them. Any buffered - /// frames are discarded, not flushed. - pub fn abort(&mut self, err: moq_net::Error) { + /// frames are discarded, not flushed. Consumes the producer. + pub fn abort(mut self, err: moq_net::Error) { self.buffer.clear(); - if let Some(mut group) = self.group.take() { + if let Some(group) = self.group.take() { let _ = group.abort(err.clone()); } let _ = self.inner.abort(err); diff --git a/rs/moq-mux/src/container/ts/export.rs b/rs/moq-mux/src/container/ts/export.rs index 09a90c6f2c..479e3fe70b 100644 --- a/rs/moq-mux/src/container/ts/export.rs +++ b/rs/moq-mux/src/container/ts/export.rs @@ -1073,6 +1073,10 @@ fn ensure_raw(container: &Container, kind: &str, name: &str) -> anyhow::Result<( // TS carries raw codec payloads, like the Legacy varint and LOC formats. Container::Legacy | Container::Loc => Ok(()), Container::Cmaf { .. } => anyhow::bail!("TS export does not support CMAF {kind} track '{name}'"), + Container::Unknown(unknown) => anyhow::bail!( + "TS export does not support container '{}' on {kind} track '{name}'", + unknown.kind().unwrap_or("") + ), } } diff --git a/rs/moq-mux/src/container/ts/import.rs b/rs/moq-mux/src/container/ts/import.rs index 55a3af4cad..732acdc0d2 100644 --- a/rs/moq-mux/src/container/ts/import.rs +++ b/rs/moq-mux/src/container/ts/import.rs @@ -311,7 +311,7 @@ impl Import { let track = crate::import::unique_track(&mut self.broadcast, ".avc3")?; Stream::H264 { split: h264::Split::new(), - import: Box::new(h264::Import::new(track, self.catalog.reserve(), Default::default())), + import: Box::new(h264::Import::new(track, self.catalog.reserve(), Default::default())?), unwrap: PtsUnwrap::default(), } } @@ -319,7 +319,7 @@ impl Import { let track = crate::import::unique_track(&mut self.broadcast, ".hev1")?; Stream::H265 { split: h265::Split::new(), - import: Box::new(h265::Import::new(track, self.catalog.reserve(), Default::default())), + import: Box::new(h265::Import::new(track, self.catalog.reserve(), Default::default())?), unwrap: PtsUnwrap::default(), } } @@ -349,7 +349,7 @@ impl Import { channel_count, }; Stream::Opus(Box::new(OpusStream { - import: opus::Import::new(track, self.catalog.reserve(), config.into()), + import: opus::Import::new(track, self.catalog.reserve(), config.into())?, unwrap: PtsUnwrap::default(), })) } @@ -593,11 +593,12 @@ impl Import { /// Abort every track with `err` instead of finishing, so subscribers see the /// real cause rather than [`moq_net::Error::Dropped`]. Buffered PES is discarded. - pub fn abort(&mut self, err: moq_net::Error) { - for stream in self.streams.values_mut() { + /// Consumes the importer. + pub fn abort(mut self, err: moq_net::Error) { + for stream in std::mem::take(&mut self.streams).into_values() { stream.abort(err.clone()); } - for section in self.sections.values_mut() { + for section in std::mem::take(&mut self.sections).into_values() { section.abort(err.clone()); } } @@ -660,7 +661,7 @@ fn register_verbatim( ); drop(guard); - Ok(catalog.media_producer(track, crate::catalog::hang::Container::Legacy)) + Ok(catalog.media_producer(track, crate::catalog::hang::Container::Legacy)?) } /// Remove a verbatim track's entry from the `mpegts` catalog section on drop. @@ -670,6 +671,18 @@ fn unregister_verbatim(catalog: &mut crate::catalog::Produc } } +/// Owns a verbatim track's `mpegts` catalog entry, removing it however the stream ends. +struct VerbatimEntry { + catalog: crate::catalog::Producer, + name: String, +} + +impl Drop for VerbatimEntry { + fn drop(&mut self) { + unregister_verbatim(&mut self.catalog, &self.name); + } +} + /// Publishes reassembled private sections (SCTE-35 and others) as verbatim frames /// on a track described in the `mpegts` catalog section. /// @@ -679,7 +692,8 @@ fn unregister_verbatim(catalog: &mut crate::catalog::Produc /// track and catalog entry and stamps each section with the media clock. struct SectionStream { track: crate::container::Producer, - catalog: crate::catalog::Producer, + /// Held for its `Drop`, which clears this track's catalog entry. + _entry: VerbatimEntry, reassembler: SectionReassembler, } @@ -699,9 +713,13 @@ impl SectionStream { catalog::Framing::Section, descriptors, )?; + let entry = VerbatimEntry { + name: track.name().to_string(), + catalog, + }; Ok(Self { track, - catalog, + _entry: entry, reassembler: SectionReassembler::default(), }) } @@ -741,18 +759,11 @@ impl SectionStream { Ok(()) } - fn abort(&mut self, err: moq_net::Error) { + fn abort(self, err: moq_net::Error) { self.track.abort(err); } } -impl Drop for SectionStream { - fn drop(&mut self) { - let name = self.track.name().to_string(); - unregister_verbatim(&mut self.catalog, &name); - } -} - /// Publishes whole reassembled PES payloads verbatim as frames on a track /// described in the `mpegts` catalog section, for elementary streams we don't decode /// (DTS audio, private PES, teletext, ...). @@ -761,7 +772,7 @@ impl Drop for SectionStream { /// type only stamps each PES payload with its (unwrapped) PTS and writes it. struct VerbatimStream { track: crate::container::Producer, - catalog: crate::catalog::Producer, + entry: VerbatimEntry, unwrap: PtsUnwrap, /// Whether the PES stream_id has been recorded into the catalog yet (once). stream_id_recorded: bool, @@ -783,9 +794,13 @@ impl VerbatimStream { catalog::Framing::Pes, descriptors, )?; + let entry = VerbatimEntry { + name: track.name().to_string(), + catalog, + }; Ok(Self { track, - catalog, + entry, unwrap: PtsUnwrap::default(), stream_id_recorded: false, }) @@ -798,7 +813,7 @@ impl VerbatimStream { // re-emits the stream under its real id (e.g. 0xBD for teletext/DVB AC-3). if !self.stream_id_recorded { let name = self.track.name().to_string(); - if let Some(mpegts) = self.catalog.lock().mpegts_mut() + if let Some(mpegts) = self.entry.catalog.lock().mpegts_mut() && let Some(verbatim) = mpegts.tracks.get_mut(&name).and_then(|t| t.verbatim.as_mut()) { verbatim.stream_id = Some(pending.stream_id); @@ -828,18 +843,11 @@ impl VerbatimStream { Ok(()) } - fn abort(&mut self, err: moq_net::Error) { + fn abort(self, err: moq_net::Error) { self.track.abort(err); } } -impl Drop for VerbatimStream { - fn drop(&mut self) { - let name = self.track.name().to_string(); - unregister_verbatim(&mut self.catalog, &name); - } -} - /// Byte-level reassembler for MPEG-TS private sections on one PID. /// /// Private sections (SCTE-35 table_id 0xFC and others) are not PES. This handles @@ -1084,7 +1092,7 @@ impl Stream { } } - fn abort(&mut self, err: moq_net::Error) { + fn abort(self, err: moq_net::Error) { match self { Stream::H264 { import, .. } => import.abort(err), Stream::H265 { import, .. } => import.abort(err), @@ -1153,7 +1161,7 @@ impl AacStream { // The importer synthesizes the AudioSpecificConfig `description` from the config so // out-of-band consumers (fMP4/MKV export, WebCodecs) can configure the decoder. let reserved = self.reserved.take().expect("aac reservation already consumed"); - let aac = aac::Import::new(track, reserved, config.into()); + let aac = aac::Import::new(track, reserved, config.into())?; self.import.insert(aac) } }; @@ -1231,8 +1239,8 @@ impl AacStream { Ok(()) } - fn abort(&mut self, err: moq_net::Error) { - if let Some(import) = &mut self.import { + fn abort(mut self, err: moq_net::Error) { + if let Some(import) = self.import.take() { import.abort(err); } } @@ -1288,7 +1296,7 @@ impl OpusStream { Ok(self.import.finish()?) } - fn abort(&mut self, err: moq_net::Error) { + fn abort(self, err: moq_net::Error) { self.import.abort(err); } } @@ -1433,7 +1441,7 @@ impl LegacyStream { let track = crate::import::unique_track(&mut self.broadcast, self.descriptor.track_suffix)?; // Consume the reservation held since the PMT: this resolves the gated rendition. let reserved = self.reserved.take().expect("legacy reservation already consumed"); - let legacy = legacy::Import::new(self.descriptor, track, reserved, config); + let legacy = legacy::Import::new(self.descriptor, track, reserved, config)?; self.import.insert(legacy) } }; @@ -1491,8 +1499,8 @@ impl LegacyStream { Ok(()) } - fn abort(&mut self, err: moq_net::Error) { - if let Some(import) = &mut self.import { + fn abort(mut self, err: moq_net::Error) { + if let Some(import) = self.import.take() { import.abort(err); } } @@ -1864,7 +1872,6 @@ mod test { bytes.extend_from_slice(&synth_pmt(&[(StreamType::Dts8ChannelLosslessAudio, 0x21)], true)); bytes.extend_from_slice(&packet(true, 0, 0, &CUE)); import.decode(&bytes).unwrap(); // must not abort on the private section - import.finish().unwrap(); assert!( import.sections.is_empty(), @@ -1877,6 +1884,7 @@ mod test { ), "the CUEI PID routes to Ignored" ); + import.finish().unwrap(); // SCTE detection takes no lock here (video/audio would still publish later): the old // discarding ScteStream took the lock and republished an empty catalog on this path. assert!( @@ -1921,7 +1929,6 @@ mod test { bytes.extend_from_slice(&synth_pmt(&[(StreamType::Dts8ChannelLosslessAudio, SECTION_PID)], true)); bytes.extend_from_slice(&packet(true, 0, 0, &CUE)); import.decode(&bytes).unwrap(); - import.finish().unwrap(); assert!( !import.streams.contains_key(&pid), @@ -1933,7 +1940,10 @@ mod test { "upgrade advertises the cue track" ); + // Finishing drops the importer, which clears its verbatim entries from the catalog, so + // read the track name first. let name = catalog.snapshot().mpegts.tracks.keys().next().unwrap().clone(); + import.finish().unwrap(); let track = consumer.track(&name).unwrap().subscribe(None).await.unwrap(); let mut reader = Consumer::new(track, Container::Legacy).with_latency(std::time::Duration::ZERO); let frame = tokio::time::timeout(std::time::Duration::from_secs(1), reader.read()) diff --git a/rs/moq-mux/src/error.rs b/rs/moq-mux/src/error.rs index cfee74cc4b..9ca1979840 100644 --- a/rs/moq-mux/src/error.rs +++ b/rs/moq-mux/src/error.rs @@ -118,10 +118,27 @@ pub enum Error { #[error("{0}")] Other(std::sync::Arc), + /// A timeline catalog section declared a timescale that isn't a valid + /// [`moq_net::Timescale`] (zero, or too large). + #[error("invalid timeline timescale: {0}")] + InvalidTimescale(u32), + /// Tried to set an application catalog section whose name collides with a /// reserved media section (`video`/`audio`). #[error("reserved catalog section: {0}")] ReservedSection(String), + + /// A rendition declared a container `kind` this build does not recognize, so its + /// frames cannot be parsed. Such a rendition must be ignored, not guessed at. + #[error("unsupported container: {0}")] + UnsupportedContainer(String), +} + +impl Error { + /// The error for a rendition whose container this build does not recognize. + pub(crate) fn unsupported_container(container: &hang::catalog::UnknownContainer) -> Self { + Self::UnsupportedContainer(container.kind().unwrap_or("").to_string()) + } } impl From for Error { diff --git a/rs/moq-mux/src/import/container.rs b/rs/moq-mux/src/import/container.rs index 2434b8ffe1..650a94e2f3 100644 --- a/rs/moq-mux/src/import/container.rs +++ b/rs/moq-mux/src/import/container.rs @@ -53,7 +53,7 @@ impl ContainerImpl { } } - fn abort(&mut self, err: moq_net::Error) { + fn abort(self, err: moq_net::Error) { match self { ContainerImpl::Fmp4(decoder) => decoder.abort(err), ContainerImpl::Mkv(decoder) => decoder.abort(err), @@ -110,8 +110,8 @@ impl Container { } /// Abort every published track with `err`, so subscribers see the real cause - /// rather than [`moq_net::Error::Dropped`]. - pub fn abort(&mut self, err: moq_net::Error) { + /// rather than [`moq_net::Error::Dropped`]. Consumes the importer. + pub fn abort(self, err: moq_net::Error) { self.inner.abort(err) } @@ -161,8 +161,8 @@ impl ContainerStream { } /// Abort every published track with `err`, so subscribers see the real cause - /// rather than [`moq_net::Error::Dropped`]. - pub fn abort(&mut self, err: moq_net::Error) { + /// rather than [`moq_net::Error::Dropped`]. Consumes the importer. + pub fn abort(self, err: moq_net::Error) { self.inner.abort(err) } diff --git a/rs/moq-mux/src/import/track.rs b/rs/moq-mux/src/import/track.rs index 1e736e1fd9..cc80670a19 100644 --- a/rs/moq-mux/src/import/track.rs +++ b/rs/moq-mux/src/import/track.rs @@ -32,7 +32,7 @@ fn build_h264_avc3( init: &[u8], hint: VideoHint, ) -> Result<(crate::codec::h264::Split, crate::codec::h264::Import)> { - let mut import = crate::codec::h264::Import::new(track, reserved, hint); + let mut import = crate::codec::h264::Import::new(track, reserved, hint)?; import.initialize(init)?; let mut split = crate::codec::h264::Split::new(); let frames = split.decode(init, None)?; @@ -49,7 +49,7 @@ fn build_h264_avc1( init: &[u8], hint: VideoHint, ) -> Result<(usize, crate::codec::h264::Import)> { - let mut import = crate::codec::h264::Import::new(track, reserved, hint); + let mut import = crate::codec::h264::Import::new(track, reserved, hint)?; import.initialize(init)?; let length_size = crate::codec::h264::Avcc::parse(init)?.length_size; Ok((length_size, import)) @@ -62,7 +62,7 @@ fn build_h265( init: &[u8], hint: VideoHint, ) -> Result<(crate::codec::h265::Split, crate::codec::h265::Import)> { - let mut import = crate::codec::h265::Import::new(track, reserved, hint); + let mut import = crate::codec::h265::Import::new(track, reserved, hint)?; import.initialize(init)?; let mut split = crate::codec::h265::Split::new(); let frames = split.decode(init, None)?; @@ -77,7 +77,7 @@ fn build_av1( init: &[u8], hint: VideoHint, ) -> Result<(crate::codec::av1::Split, crate::codec::av1::Import)> { - let mut import = crate::codec::av1::Import::new(track, reserved, hint); + let mut import = crate::codec::av1::Import::new(track, reserved, hint)?; import.initialize(init)?; let mut split = crate::codec::av1::Split::new(); // av1C (leading 0x81, ISO/IEC 14496-15) is an out-of-band config record, not an @@ -166,12 +166,12 @@ impl Track { } "vp8" | "vp08" => { let mut import = - crate::codec::vp8::Import::new(track, reserved, video_hint(&init, Some(VideoCodec::VP8))); + crate::codec::vp8::Import::new(track, reserved, video_hint(&init, Some(VideoCodec::VP8)))?; import.initialize(data)?; TrackKind::Vp8(import) } "vp9" | "vp09" => { - let mut import = crate::codec::vp9::Import::new(track, reserved, video_hint(&init, None)); + let mut import = crate::codec::vp9::Import::new(track, reserved, video_hint(&init, None))?; import.initialize(data)?; TrackKind::Vp9(import) } @@ -179,20 +179,20 @@ impl Track { // OpusHead, AudioSpecificConfig, ...); `codec::config` errors when they're missing or bad. "aac" => { let config = crate::codec::aac::config(data)?; - TrackKind::Aac(crate::codec::aac::Import::new(track, reserved, config)) + TrackKind::Aac(crate::codec::aac::Import::new(track, reserved, config)?) } "opus" => { let config = crate::codec::opus::config(data)?; - TrackKind::Opus(crate::codec::opus::Import::new(track, reserved, config)) + TrackKind::Opus(crate::codec::opus::Import::new(track, reserved, config)?) } "flac" => { // `data` is a FLAC header: the `fLaC` marker plus the STREAMINFO block. let config = crate::codec::flac::config(data)?; - TrackKind::Flac(crate::codec::flac::Import::new(track, reserved, config)) + TrackKind::Flac(crate::codec::flac::Import::new(track, reserved, config)?) } "mp3" => { let config = crate::codec::mp3::config(data)?; - TrackKind::Mp3(crate::codec::mp3::Import::new(track, reserved, config)) + TrackKind::Mp3(crate::codec::mp3::Import::new(track, reserved, config)?) } _ => return Err(crate::Error::UnknownFormat(init.format)), }; @@ -265,19 +265,19 @@ impl Track { } /// Abort the track with `err` instead of finishing it cleanly, so subscribers - /// see the real cause rather than [`moq_net::Error::Dropped`]. - pub fn abort(&mut self, err: moq_net::Error) { + /// see the real cause rather than [`moq_net::Error::Dropped`]. Consumes the importer. + pub fn abort(self, err: moq_net::Error) { match self.kind { - TrackKind::Avc3 { ref mut import, .. } => import.abort(err), - TrackKind::Avc1 { ref mut import, .. } => import.abort(err), - TrackKind::Hev1 { ref mut import, .. } => import.abort(err), - TrackKind::Av01 { ref mut import, .. } => import.abort(err), - TrackKind::Vp8(ref mut import) => import.abort(err), - TrackKind::Vp9(ref mut import) => import.abort(err), - TrackKind::Aac(ref mut import) => import.abort(err), - TrackKind::Opus(ref mut import) => import.abort(err), - TrackKind::Mp3(ref mut import) => import.abort(err), - TrackKind::Flac(ref mut import) => import.abort(err), + TrackKind::Avc3 { import, .. } => import.abort(err), + TrackKind::Avc1 { import, .. } => import.abort(err), + TrackKind::Hev1 { import, .. } => import.abort(err), + TrackKind::Av01 { import, .. } => import.abort(err), + TrackKind::Vp8(import) => import.abort(err), + TrackKind::Vp9(import) => import.abort(err), + TrackKind::Aac(import) => import.abort(err), + TrackKind::Opus(import) => import.abort(err), + TrackKind::Mp3(import) => import.abort(err), + TrackKind::Flac(import) => import.abort(err), } } @@ -423,15 +423,15 @@ impl TrackStream { let kind = match init.format.as_str() { "avc3" | "h264" => TrackStreamKind::Avc3 { split: crate::codec::h264::Split::new(), - import: crate::codec::h264::Import::new(track, reserved, hint), + import: crate::codec::h264::Import::new(track, reserved, hint)?, }, "hev1" => TrackStreamKind::Hev1 { split: crate::codec::h265::Split::new(), - import: crate::codec::h265::Import::new(track, reserved, hint), + import: crate::codec::h265::Import::new(track, reserved, hint)?, }, "av01" | "av1" | "av1c" | "av1C" => TrackStreamKind::Av01 { split: crate::codec::av1::Split::new(), - import: crate::codec::av1::Import::new(track, reserved, hint), + import: crate::codec::av1::Import::new(track, reserved, hint)?, }, _ => return Err(crate::Error::UnknownFormat(init.format)), }; @@ -541,12 +541,12 @@ impl TrackStream { } /// Abort the track with `err` instead of finishing it cleanly, so subscribers - /// see the real cause rather than [`moq_net::Error::Dropped`]. - pub fn abort(&mut self, err: moq_net::Error) { + /// see the real cause rather than [`moq_net::Error::Dropped`]. Consumes the importer. + pub fn abort(self, err: moq_net::Error) { match self.kind { - TrackStreamKind::Avc3 { ref mut import, .. } => import.abort(err), - TrackStreamKind::Hev1 { ref mut import, .. } => import.abort(err), - TrackStreamKind::Av01 { ref mut import, .. } => import.abort(err), + TrackStreamKind::Avc3 { import, .. } => import.abort(err), + TrackStreamKind::Hev1 { import, .. } => import.abort(err), + TrackStreamKind::Av01 { import, .. } => import.abort(err), } } @@ -711,7 +711,7 @@ mod tests { sample_rate: 48_000, channel_count: 2, }; - let mut import = crate::codec::opus::Import::new(track, catalog.reserve(), config.into()); + let mut import = crate::codec::opus::Import::new(track, catalog.reserve(), config.into()).unwrap(); assert!(catalog.snapshot().audio.renditions.contains_key("audio")); let mut media = crate::container::Consumer::new(subscriber, crate::catalog::hang::Container::Legacy); diff --git a/rs/moq-mux/src/select.rs b/rs/moq-mux/src/select.rs index e4efb73a05..e6e4cd7c4d 100644 --- a/rs/moq-mux/src/select.rs +++ b/rs/moq-mux/src/select.rs @@ -4,7 +4,8 @@ //! additive: a default [`Broadcast`] selects *nothing*, and you opt a role in with //! [`video`](Broadcast::video) / [`audio`](Broadcast::audio). Within an opted-in //! role, an empty field matches everything; listing values keeps renditions matching -//! any one of them (a union within a field, intersected across fields). +//! any one of them (a union within a field, intersected across fields). A rendition whose +//! container `kind` this build does not recognize is never selected, per the hang spec. //! //! The same [`Broadcast`] drives selection at either end of the pipeline: narrowing //! a published catalog on the consume side (see [`catalog::Select`](crate::catalog::Select)), @@ -12,7 +13,7 @@ use hang::catalog::{AudioCodecKind, AudioConfig, VideoCodecKind, VideoConfig}; -use crate::catalog::hang::{Catalog, CatalogExt}; +use crate::catalog::hang::{Catalog, CatalogExt, supported}; /// Which renditions of a broadcast to keep. /// @@ -87,7 +88,8 @@ impl Video { } fn matches(&self, name: &str, config: &VideoConfig) -> bool { - (self.name.is_empty() || self.name.iter().any(|n| n == name)) + supported(name, &config.container) + && (self.name.is_empty() || self.name.iter().any(|n| n == name)) && (self.codec.is_empty() || self.codec.contains(&config.codec.kind())) } } @@ -113,7 +115,8 @@ impl Audio { } fn matches(&self, name: &str, config: &AudioConfig) -> bool { - (self.name.is_empty() || self.name.iter().any(|n| n == name)) + supported(name, &config.container) + && (self.name.is_empty() || self.name.iter().any(|n| n == name)) && (self.codec.is_empty() || self.codec.contains(&config.codec.kind())) } } @@ -234,6 +237,16 @@ mod tests { assert_eq!(video_names(&catalog), vec!["a", "b"]); } + #[test] + fn unknown_container_never_selected() { + let (name, mut future) = h264("future"); + future.container = serde_json::from_str(r#"{"kind":"future"}"#).unwrap(); + + let mut catalog = catalog(vec![h264("known"), (name, future)], vec![]); + Broadcast::default().video(Video::default()).retain(&mut catalog); + assert_eq!(video_names(&catalog), vec!["known"]); + } + #[test] fn name_and_codec_intersect() { let mut catalog = catalog(vec![h264("hi"), vp9("hi2"), h264("lo")], vec![]); diff --git a/rs/moq-mux/src/timeline.rs b/rs/moq-mux/src/timeline.rs index a7c798e2fe..75bd8c5d9b 100644 --- a/rs/moq-mux/src/timeline.rs +++ b/rs/moq-mux/src/timeline.rs @@ -23,7 +23,9 @@ //! //! On the read side, [`Consumer::subscribe`] reads a timeline straight from its //! [`hang::catalog::Timeline`] section (so the track name and timescale come from the catalog and -//! can't be mismatched) and yields decoded [`Entry`]s with a real [`Timestamp`]. +//! can't be mismatched) and yields decoded [`Entry`]s with a real [`Timestamp`]. It is generic over +//! a [`RecordExt`], so it can read the extra fields another publisher flattens into a record; the +//! write side publishes the base record shape only. //! //! On the wire the track is a DEFLATE-compressed [`moq_json::stream`] (a single group, one record //! per frame; see [`hang::timeline`] for the record schema). @@ -50,13 +52,14 @@ pub const DEFAULT_GRANULARITY: Timestamp = Timestamp::new_const(1, Timescale::SE /// A media timeline: its catalog [`section`](Self::section) and wall anchor, and the [`Recorder`] /// its group opens are recorded through. /// -/// Generic over the record extension `E` (defaulting to `()`; see [`RecordExt`]). `Clone`, and every -/// clone shares the one track and its wall anchor, so a set of aligned renditions can advertise one -/// timeline. Get one from [`catalog::Producer::timeline`](crate::catalog::Producer::timeline), which -/// keeps ownership and closes the track when the catalog finishes. +/// Publishes the base [`Record`] shape (no extension); a [`Consumer`] can still read a record +/// extension published by another implementation. `Clone`, and every clone shares the one track and +/// its wall anchor, so a set of aligned renditions can advertise one timeline. Get one from +/// [`catalog::Producer::timeline`](crate::catalog::Producer::timeline), which keeps ownership and +/// closes the track when the catalog finishes. #[derive(Clone)] -pub struct Producer { - inner: moq_json::stream::Producer>, +pub struct Producer { + inner: moq_json::stream::Producer, track: String, timescale: Timescale, granularity: Timestamp, @@ -65,7 +68,7 @@ pub struct Producer { wall: Arc>>, } -impl Producer { +impl Producer { /// Create a timeline track for the media rendition `name` on the given broadcast. /// /// The track is named per [`hang::timeline::track_name`] (`.timeline.z`) at the @@ -125,7 +128,7 @@ impl Producer { /// Wire it into a media track's [`container::Producer`](crate::container::Producer) with /// [`with_recorder`](crate::container::Producer::with_recorder). A recorder owns its own throttle /// cursor, so wire exactly one per timeline (a shared timeline is filled by its source alone). - pub fn recorder(&self) -> Recorder { + pub fn recorder(&self) -> Recorder { Recorder { inner: self.inner.clone(), timescale: self.timescale, @@ -149,15 +152,15 @@ impl Producer { /// Move-only (not `Clone`): it owns its throttle cursor, so wire exactly one per timeline. Minted by /// [`Producer::recorder`] and held by a rendition's /// [`container::Producer`](crate::container::Producer). -pub struct Recorder { - inner: moq_json::stream::Producer>, +pub struct Recorder { + inner: moq_json::stream::Producer, timescale: Timescale, granularity: Timestamp, // The pts of the last recorded group; the throttle floor. Owned, since a recorder is 1:1. last: Option, } -impl Recorder { +impl Recorder { /// Record that group `sequence` opened at presentation time `pts`, unless it falls within the /// granularity of the last recorded group (skipped, so a consumer extrapolates or fetches). pub(crate) fn record(&mut self, sequence: u64, pts: Timestamp) -> Result<(), moq_net::Error> { @@ -208,40 +211,44 @@ impl Consumer { /// /// The section supplies both the track name and the timescale, so a reader can't pair the wrong /// scale with the track. - pub async fn subscribe( - broadcast: &moq_net::broadcast::Consumer, - section: &Timeline, - ) -> Result { + /// + /// Errors if the section declares a timescale that isn't representable. + pub async fn subscribe(broadcast: &moq_net::broadcast::Consumer, section: &Timeline) -> crate::Result { let track = broadcast.track(§ion.track)?.subscribe(None).await?; let config = moq_json::stream::ConsumerConfig::default().with_compression(true); Ok(Self { inner: moq_json::stream::Consumer::new(track, config), - timescale: Timescale::new(section.timescale as u64).unwrap_or(Timescale::MILLI), + timescale: Timescale::new(section.timescale as u64) + .map_err(|_| crate::Error::InvalidTimescale(section.timescale))?, }) } - fn decode(&self, record: Record) -> Entry { - Entry { + /// Decode a record into an entry, converting its pts out of the wire timescale. + /// + /// A pts the timescale can't represent is an error rather than a substituted value: silently + /// moving a timestamp would misdirect seeking and live-edge logic. + fn decode(&self, record: Record) -> crate::Result> { + Ok(Entry { group: record.group, - pts: Timestamp::new(record.pts, self.timescale).unwrap_or(Timestamp::ZERO), + pts: Timestamp::new(record.pts, self.timescale)?, ext: record.ext, - } + }) } /// Get the next entry, or `None` once the track ends. - pub async fn next(&mut self) -> Result>, moq_json::Error> { + pub async fn next(&mut self) -> crate::Result>> { match self.inner.next().await? { - Some(record) => Ok(Some(self.decode(record))), + Some(record) => Ok(Some(self.decode(record)?)), None => Ok(None), } } /// Poll for the next entry, without blocking. - pub fn poll_next(&mut self, waiter: &kio::Waiter) -> Poll>, moq_json::Error>> { + pub fn poll_next(&mut self, waiter: &kio::Waiter) -> Poll>>> { match self.inner.poll_next(waiter)? { - Poll::Ready(Some(record)) => Poll::Ready(Ok(Some(self.decode(record)))), + Poll::Ready(Some(record)) => Poll::Ready(self.decode(record).map(Some)), Poll::Ready(None) => Poll::Ready(Ok(None)), Poll::Pending => Poll::Pending, } @@ -287,7 +294,7 @@ mod test { #[tokio::test] async fn records_group_opens_in_milliseconds() { let mut broadcast = moq_net::broadcast::Info::new().produce(); - let mut timeline = Producer::new(&mut broadcast, "video0").unwrap(); + let timeline = Producer::new(&mut broadcast, "video0").unwrap(); assert_eq!(timeline.track, "video0.timeline.z"); let track = broadcast.create_track("video0", None).unwrap(); @@ -298,7 +305,7 @@ mod test { media.write(frame(2_000_000, false)).unwrap(); // extends group 0 media.write(frame(4_000_000, true)).unwrap(); // group 1 @ 4_000_000us media.finish().unwrap(); - timeline.finish().unwrap(); + timeline.clone().finish().unwrap(); // Entry pts is a real Timestamp (decoded from the ms-timescale record). assert_eq!(drain(&broadcast, &timeline).await, vec![entry(0, 0), entry(1, 4_000)]); @@ -307,7 +314,7 @@ mod test { #[tokio::test] async fn granularity_throttles_records() { let mut broadcast = moq_net::broadcast::Info::new().produce(); - let mut timeline = Producer::new(&mut broadcast, "audio0").unwrap(); + let timeline = Producer::new(&mut broadcast, "audio0").unwrap(); let mut recorder = timeline.recorder(); // Default granularity is 1s. Group opens 300ms apart, all within a second of the first, then @@ -316,7 +323,7 @@ mod test { recorder.record(seq, Timestamp::from_millis(ms).unwrap()).unwrap(); } drop(recorder); - timeline.finish().unwrap(); + timeline.clone().finish().unwrap(); assert_eq!(drain(&broadcast, &timeline).await, vec![entry(0, 0), entry(4, 1200)]); } @@ -324,7 +331,7 @@ mod test { #[test] fn section_advertises_track_and_wall() { let mut broadcast = moq_net::broadcast::Info::new().produce(); - let mut timeline = Producer::<()>::new(&mut broadcast, "audio0").unwrap(); + let mut timeline = Producer::new(&mut broadcast, "audio0").unwrap(); let section = timeline.section(); assert_eq!(section.track, "audio0.timeline.z"); @@ -343,15 +350,55 @@ mod test { assert_eq!(timeline.section().wall, Some(1_751_846_400_000 - moq - 2_000)); } + #[tokio::test] + async fn rejects_an_invalid_timescale() { + let mut broadcast = moq_net::broadcast::Info::new().produce(); + let timeline = Producer::new(&mut broadcast, "video0").unwrap(); + timeline.clone().finish().unwrap(); + + // A timescale of 0 can't be honored, and quietly reading the track at milliseconds would + // report timestamps the publisher never meant. + let mut section = timeline.section(); + section.timescale = 0; + match Consumer::<()>::subscribe(&broadcast.consume(), §ion).await { + Err(crate::Error::InvalidTimescale(0)) => {} + Err(err) => panic!("expected an invalid timescale, got {err:?}"), + Ok(_) => panic!("expected an invalid timescale to be rejected"), + } + } + + #[tokio::test] + async fn rejects_an_out_of_range_pts() { + let mut broadcast = moq_net::broadcast::Info::new().produce(); + let timeline = Producer::new(&mut broadcast, "video0").unwrap(); + + // Publish a record whose pts no Timestamp can hold, bypassing the recorder. + let track = broadcast.create_track("raw.timeline.z", None).unwrap(); + let config = moq_json::stream::ProducerConfig::default().with_compression(true); + let mut raw = moq_json::stream::Producer::new(track, config); + raw.append(&Record::<()>::new(0, u64::MAX)).unwrap(); + raw.finish().unwrap(); + + let mut section = timeline.section(); + section.track = "raw.timeline.z".to_string(); + let mut consumer = Consumer::<()>::subscribe(&broadcast.consume(), §ion).await.unwrap(); + + let waiter = kio::Waiter::noop(); + match consumer.poll_next(&waiter) { + Poll::Ready(Err(crate::Error::TimestampOverflow(_))) => {} + other => panic!("expected a decode error, got {other:?}"), + } + } + #[tokio::test] async fn consumer_decodes_pts_from_the_section() { let mut broadcast = moq_net::broadcast::Info::new().produce(); - let mut timeline = Producer::new(&mut broadcast, "video0").unwrap(); + let timeline = Producer::new(&mut broadcast, "video0").unwrap(); timeline .recorder() .record(3, Timestamp::from_micros(7_000).unwrap()) .unwrap(); - timeline.finish().unwrap(); + timeline.clone().finish().unwrap(); // The reader takes the track name + timescale from the section, and yields a real Timestamp. // The pts is decoded at the timeline's (millisecond) timescale, so compare the instant rather diff --git a/rs/moq-net/src/model/broadcast.rs b/rs/moq-net/src/model/broadcast.rs index e15cd35d0a..d90f638f89 100644 --- a/rs/moq-net/src/model/broadcast.rs +++ b/rs/moq-net/src/model/broadcast.rs @@ -1214,7 +1214,7 @@ mod test { // Subscribe to a track that doesn't exist yet, then serve it. let c1_fut = subscribe_pending!(bc, "unknown_track"); - let mut producer1 = broadcast.assert_request().accept(None); + let producer1 = broadcast.assert_request().accept(None); let consumer1 = c1_fut.await.unwrap(); // The producer should NOT be unused yet because there's a consumer. diff --git a/rs/moq-net/src/model/group.rs b/rs/moq-net/src/model/group.rs index 7a06d2beeb..bad153cdb6 100644 --- a/rs/moq-net/src/model/group.rs +++ b/rs/moq-net/src/model/group.rs @@ -381,7 +381,7 @@ impl Producer { /// Fail the group because an in-flight frame couldn't complete (called by /// [`frame::Producer::abort`] / its drop). pub(crate) fn frame_abort(&mut self, err: Error) { - let _ = self.abort(err); + let _ = self.clone().abort(err); } /// Return the number of frames written so far (completed plus any in-flight). @@ -391,6 +391,9 @@ impl Producer { } /// Mark the group as complete; no more frames will be written. + /// + /// Borrows rather than consumes, so a later failure can still be reported through + /// [`abort`](Self::abort). The handle also keeps the cached frames readable. pub fn finish(&mut self) -> Result<()> { let mut state = modify(&self.state)?; state.fin = true; @@ -399,10 +402,10 @@ impl Producer { /// Abort the group with the given error. /// - /// No updates can be made after this point. Drops the cached frames so a stale - /// [`Consumer`] can't pin their buffers in memory forever; consumers that haven't - /// drained yet surface the abort error instead of the leftover cache. - pub fn abort(&mut self, err: Error) -> Result<()> { + /// Consumes the handle. Drops the cached frames so a stale [`Consumer`] can't pin + /// their buffers in memory forever; consumers that haven't drained yet surface the + /// abort error instead of the leftover cache. + pub fn abort(self, err: Error) -> Result<()> { let mut guard = modify(&self.state)?; guard.abort = Some(err); guard.release(); @@ -880,7 +883,7 @@ mod test { #[test] fn abort_propagates() { - let mut producer = Info { sequence: 0 }.produce(); + let producer = Info { sequence: 0 }.produce(); let mut consumer = producer.consume(); producer.abort(crate::Error::Cancel).unwrap(); @@ -899,7 +902,7 @@ mod test { let _consumer = producer.consume(); assert_eq!(producer.state.read().frames.len(), 1); - producer.abort(crate::Error::Cancel).unwrap(); + producer.clone().abort(crate::Error::Cancel).unwrap(); let state = producer.state.read(); assert!(state.frames.is_empty(), "cached frames should be dropped on abort"); diff --git a/rs/moq-net/src/model/origin.rs b/rs/moq-net/src/model/origin.rs index fd4f0eeba7..d2b21c50bd 100644 --- a/rs/moq-net/src/model/origin.rs +++ b/rs/moq-net/src/model/origin.rs @@ -2909,8 +2909,8 @@ mod tests { // Source A dies (session loss): the track re-splices from B and nothing // is announced. + // abort() consumes the producer, so this both aborts and drops it. producer.abort(Error::Dropped).unwrap(); - drop(producer); source_a.abort(); drop(source_a); drop(dynamic_a); diff --git a/rs/moq-net/src/model/track.rs b/rs/moq-net/src/model/track.rs index 5a516820df..dd8f494a26 100644 --- a/rs/moq-net/src/model/track.rs +++ b/rs/moq-net/src/model/track.rs @@ -460,13 +460,13 @@ impl TrackState { } self.duplicates.remove(&group.sequence); - // Abort the group before dropping it so any consumer still reading it - // surfaces `Error::Old` instead of blocking forever on a frame that will - // never arrive (the cached producer is about to be gone). Without this a - // reader parked on an aged-out group hangs indefinitely, since the group - // was never finished or aborted -- it just silently disappeared. - let _ = group.abort(Error::Old); - *slot = None; + // Take the group out of the cache and abort it, so any consumer still reading + // surfaces `Error::Old` instead of blocking forever on a frame that will never + // arrive. Without this a reader parked on an aged-out group hangs indefinitely, + // since the group is neither finished nor aborted. + if let Some((group, _)) = slot.take() { + let _ = group.abort(Error::Old); + } } // Trim leading tombstones to advance the offset. @@ -841,12 +841,15 @@ impl Producer { /// Abort the track with the given error. /// - /// Drops the cached groups so a stale [`Consumer`] can't pin them (and - /// their frame buffers) in memory forever. Consumers that haven't drained yet - /// surface the abort error instead of the leftover cache. Child groups are - /// independent: a consumer that already pulled a [`group::Consumer`] keeps its - /// own handle and can finish reading it. - pub fn abort(&mut self, err: Error) -> Result<()> { + /// Consumes the handle, since nothing can be written to an aborted track. Drops the + /// cached groups so a stale [`Consumer`] can't pin them (and their frame buffers) in + /// memory forever. Consumers that haven't drained yet surface the abort error instead + /// of the leftover cache. Child groups are independent: a consumer that already pulled + /// a [`group::Consumer`] keeps its own handle and can finish reading it. + /// + /// [`finish`](Self::finish) is deliberately not terminal: it declares the final + /// sequence, and lower-numbered groups may still be written afterwards. + pub fn abort(self, err: Error) -> Result<()> { let mut guard = self.modify()?; guard.abort = Some(err); guard.groups.clear(); @@ -2952,7 +2955,7 @@ mod test { let mut consumer = producer.subscribe(None); assert_eq!(live_groups(&producer.state.read()), 2); - producer.abort(Error::Cancel).unwrap(); + producer.clone().abort(Error::Cancel).unwrap(); { let state = producer.state.read(); @@ -3952,7 +3955,7 @@ mod test { tokio::time::advance(Duration::from_millis(10)).await; // The publisher aborts its own latest group; the slot stays at max_sequence. - let mut latest = producer.append_group().unwrap(); // seq 1 + let latest = producer.append_group().unwrap(); // seq 1 latest.abort(Error::Cancel).unwrap(); tokio::time::advance(Duration::from_millis(10)).await; @@ -4023,7 +4026,7 @@ mod test { #[tokio::test] async fn fetch_aborts_with_track() { - let mut producer = track_producer("test", None); + let producer = track_producer("test", None); let dynamic = producer.dynamic(); let consumer = producer.consume(); diff --git a/rs/moq-rtc/src/codec/av1.rs b/rs/moq-rtc/src/codec/av1.rs index e38680ea00..e5682a0c6a 100644 --- a/rs/moq-rtc/src/codec/av1.rs +++ b/rs/moq-rtc/src/codec/av1.rs @@ -16,7 +16,7 @@ impl Bridge { /// Publish an `.av1` track on `broadcast`, adding the catalog rendition once config is known. pub fn new(mut broadcast: moq_net::broadcast::Producer, catalog: moq_mux::catalog::Producer) -> Result { let track = moq_mux::import::unique_track(&mut broadcast, ".av1")?; - let import = moq_mux::codec::av1::Import::new(track, catalog.reserve(), Default::default()); + let import = moq_mux::codec::av1::Import::new(track, catalog.reserve(), Default::default())?; let split = moq_mux::codec::av1::Split::new(); Ok(Self { split, import }) } @@ -33,7 +33,7 @@ impl codec::Bridge for Bridge { Ok(()) } - fn abort(&mut self, err: moq_net::Error) { + fn abort(self: Box, err: moq_net::Error) { self.import.abort(err); } } diff --git a/rs/moq-rtc/src/codec/h264.rs b/rs/moq-rtc/src/codec/h264.rs index 891d8a69ee..c438d2d9cd 100644 --- a/rs/moq-rtc/src/codec/h264.rs +++ b/rs/moq-rtc/src/codec/h264.rs @@ -15,7 +15,7 @@ pub struct Bridge { impl Bridge { pub fn new(mut broadcast: moq_net::broadcast::Producer, catalog: moq_mux::catalog::Producer) -> Result { let track = moq_mux::import::unique_track(&mut broadcast, ".avc3")?; - let import = moq_mux::codec::h264::Import::new(track, catalog.reserve(), Default::default()); + let import = moq_mux::codec::h264::Import::new(track, catalog.reserve(), Default::default())?; let split = moq_mux::codec::h264::Split::new(); Ok(Self { split, import }) } @@ -32,7 +32,7 @@ impl codec::Bridge for Bridge { Ok(()) } - fn abort(&mut self, err: moq_net::Error) { + fn abort(self: Box, err: moq_net::Error) { self.import.abort(err); } } diff --git a/rs/moq-rtc/src/codec/h265.rs b/rs/moq-rtc/src/codec/h265.rs index 2b4e5b18b1..33441bc411 100644 --- a/rs/moq-rtc/src/codec/h265.rs +++ b/rs/moq-rtc/src/codec/h265.rs @@ -17,7 +17,7 @@ impl Bridge { /// Publish a `.hev1` track on `broadcast`, adding the catalog rendition once config is known. pub fn new(mut broadcast: moq_net::broadcast::Producer, catalog: moq_mux::catalog::Producer) -> Result { let track = moq_mux::import::unique_track(&mut broadcast, ".hev1")?; - let import = moq_mux::codec::h265::Import::new(track, catalog.reserve(), Default::default()); + let import = moq_mux::codec::h265::Import::new(track, catalog.reserve(), Default::default())?; let split = moq_mux::codec::h265::Split::new(); Ok(Self { split, import }) } @@ -34,7 +34,7 @@ impl codec::Bridge for Bridge { Ok(()) } - fn abort(&mut self, err: moq_net::Error) { + fn abort(self: Box, err: moq_net::Error) { self.import.abort(err); } } diff --git a/rs/moq-rtc/src/codec/mod.rs b/rs/moq-rtc/src/codec/mod.rs index 97aa59baff..3d386de351 100644 --- a/rs/moq-rtc/src/codec/mod.rs +++ b/rs/moq-rtc/src/codec/mod.rs @@ -46,7 +46,24 @@ pub trait Bridge: Send { /// Abort the published track with `err` so subscribers see the real cause /// (the peer disconnected, an ICE failure) rather than a bare `Error::Dropped`. - fn abort(&mut self, err: moq_net::Error); + /// + /// Consumes the bridge: the track is dead afterwards. + fn abort(self: Box, err: moq_net::Error); +} + +/// A bridge's video catalog entry, removed however the bridge ends. +/// +/// A separate value rather than a `Drop` on the bridge itself, so a bridge's terminal +/// [`Bridge::abort`] can consume its track producer. +pub(crate) struct VideoRendition { + pub catalog: moq_mux::catalog::Producer, + pub name: String, +} + +impl Drop for VideoRendition { + fn drop(&mut self) { + self.catalog.lock().video.renditions.remove(&self.name); + } } /// One RTP-ready codec frame produced by an egress [`Track`]. diff --git a/rs/moq-rtc/src/codec/opus.rs b/rs/moq-rtc/src/codec/opus.rs index c78130fdbb..05bda79f0e 100644 --- a/rs/moq-rtc/src/codec/opus.rs +++ b/rs/moq-rtc/src/codec/opus.rs @@ -21,7 +21,7 @@ impl Bridge { channel_count, }; let track = moq_mux::import::unique_track(&mut broadcast, ".opus")?; - let import = moq_mux::codec::opus::Import::new(track, catalog.reserve(), config.into()); + let import = moq_mux::codec::opus::Import::new(track, catalog.reserve(), config.into())?; Ok(Self { import }) } } @@ -34,7 +34,7 @@ impl codec::Bridge for Bridge { Ok(()) } - fn abort(&mut self, err: moq_net::Error) { + fn abort(self: Box, err: moq_net::Error) { self.import.abort(err); } } diff --git a/rs/moq-rtc/src/codec/vp8.rs b/rs/moq-rtc/src/codec/vp8.rs index 739e040c2f..1c582eded4 100644 --- a/rs/moq-rtc/src/codec/vp8.rs +++ b/rs/moq-rtc/src/codec/vp8.rs @@ -8,7 +8,8 @@ use crate::{Result, codec}; /// Forwards str0m's VP8 frames to a `.vp8` track, detecting keyframes inline. pub struct Bridge { - catalog: moq_mux::catalog::Producer, + /// Owns the catalog rendition, retiring it when the bridge goes away. + rendition: codec::VideoRendition, track: moq_mux::container::Producer, announced: bool, } @@ -17,30 +18,32 @@ impl Bridge { /// Publish a `.vp8` track on `broadcast`; the catalog rendition is added on the first frame. pub fn new(mut broadcast: moq_net::broadcast::Producer, catalog: moq_mux::catalog::Producer) -> Result { let track = broadcast.create_track(broadcast.unique_name(".vp8"), hang::container::track_info())?; - let producer = catalog.media_producer(track, moq_mux::catalog::hang::Container::Legacy); + let name = track.name().to_string(); + let producer = catalog.media_producer(track, moq_mux::catalog::hang::Container::Legacy)?; Ok(Self { - catalog, + rendition: codec::VideoRendition { catalog, name }, track: producer, announced: false, }) } - fn announce(&mut self) { + fn announce(&mut self) -> Result<()> { if self.announced { - return; + return Ok(()); } - let name = self.track.track().name().to_string(); + let name = self.rendition.name.clone(); let mut config = hang::catalog::VideoConfig::new(hang::catalog::VideoCodec::VP8); config.container = hang::catalog::Container::Legacy; - config.timeline = Some(self.catalog.timeline(&name).section()); - self.catalog.lock().video.renditions.insert(name, config); + config.timeline = Some(self.rendition.catalog.timeline(&name)?.section()); + self.rendition.catalog.lock().video.renditions.insert(name, config); self.announced = true; + Ok(()) } } impl codec::Bridge for Bridge { fn push(&mut self, frame: codec::Frame) -> Result<()> { - self.announce(); + self.announce()?; let pts = moq_net::Timestamp::from_micros(frame.timestamp_us) .map_err(|err| crate::Error::Other(anyhow::anyhow!("invalid timestamp: {err}")))?; // VP8: first byte bit 0 == 0 means keyframe (RFC 6386 §9.1). @@ -56,13 +59,7 @@ impl codec::Bridge for Bridge { Ok(()) } - fn abort(&mut self, err: moq_net::Error) { + fn abort(self: Box, err: moq_net::Error) { self.track.abort(err); } } - -impl Drop for Bridge { - fn drop(&mut self) { - self.catalog.lock().video.renditions.remove(self.track.track().name()); - } -} diff --git a/rs/moq-rtc/src/codec/vp9.rs b/rs/moq-rtc/src/codec/vp9.rs index 0410504d06..cc275bfbfe 100644 --- a/rs/moq-rtc/src/codec/vp9.rs +++ b/rs/moq-rtc/src/codec/vp9.rs @@ -7,7 +7,8 @@ use crate::{Result, codec}; /// Forwards str0m's VP9 frames to a `.vp9` track, detecting keyframes inline. pub struct Bridge { - catalog: moq_mux::catalog::Producer, + /// Owns the catalog rendition, retiring it when the bridge goes away. + rendition: codec::VideoRendition, track: moq_mux::container::Producer, announced: bool, } @@ -16,30 +17,32 @@ impl Bridge { /// Publish a `.vp9` track on `broadcast`; the catalog rendition is added on the first frame. pub fn new(mut broadcast: moq_net::broadcast::Producer, catalog: moq_mux::catalog::Producer) -> Result { let track = broadcast.create_track(broadcast.unique_name(".vp9"), hang::container::track_info())?; - let producer = catalog.media_producer(track, moq_mux::catalog::hang::Container::Legacy); + let name = track.name().to_string(); + let producer = catalog.media_producer(track, moq_mux::catalog::hang::Container::Legacy)?; Ok(Self { - catalog, + rendition: codec::VideoRendition { catalog, name }, track: producer, announced: false, }) } - fn announce(&mut self) { + fn announce(&mut self) -> Result<()> { if self.announced { - return; + return Ok(()); } - let name = self.track.track().name().to_string(); + let name = self.rendition.name.clone(); let mut config = hang::catalog::VideoConfig::new(hang::catalog::VP9::default()); config.container = hang::catalog::Container::Legacy; - config.timeline = Some(self.catalog.timeline(&name).section()); - self.catalog.lock().video.renditions.insert(name, config); + config.timeline = Some(self.rendition.catalog.timeline(&name)?.section()); + self.rendition.catalog.lock().video.renditions.insert(name, config); self.announced = true; + Ok(()) } } impl codec::Bridge for Bridge { fn push(&mut self, frame: codec::Frame) -> Result<()> { - self.announce(); + self.announce()?; let pts = moq_net::Timestamp::from_micros(frame.timestamp_us) .map_err(|err| crate::Error::Other(anyhow::anyhow!("invalid timestamp: {err}")))?; let keyframe = is_keyframe(&frame.payload); @@ -54,7 +57,7 @@ impl codec::Bridge for Bridge { Ok(()) } - fn abort(&mut self, err: moq_net::Error) { + fn abort(self: Box, err: moq_net::Error) { self.track.abort(err); } } @@ -83,12 +86,6 @@ fn is_keyframe(payload: &[u8]) -> bool { frame_type == 0 } -impl Drop for Bridge { - fn drop(&mut self) { - self.catalog.lock().video.renditions.remove(self.track.track().name()); - } -} - #[cfg(test)] mod tests { use super::is_keyframe; diff --git a/rs/moq-rtc/src/session.rs b/rs/moq-rtc/src/session.rs index 70e32640f9..b899d678d3 100644 --- a/rs/moq-rtc/src/session.rs +++ b/rs/moq-rtc/src/session.rs @@ -544,8 +544,10 @@ impl Bridges { /// Abort every bridge's track with `err` so subscribers see the real cause /// rather than a bare `Error::Dropped`. + /// + /// Aborting consumes each bridge, so the map is emptied: the session is over. pub fn abort(&mut self, err: moq_net::Error) { - for bridge in self.inner.values_mut() { + for bridge in std::mem::take(&mut self.inner).into_values() { bridge.abort(err.clone()); } } diff --git a/rs/moq-rtmp/src/dial.rs b/rs/moq-rtmp/src/dial.rs index 1cd0013702..f7bca26164 100644 --- a/rs/moq-rtmp/src/dial.rs +++ b/rs/moq-rtmp/src/dial.rs @@ -481,7 +481,9 @@ impl Publisher { /// Abort the published tracks with `err` so subscribers see the real cause /// (the remote dropped, a protocol error) rather than a generic `Error::Dropped`. - fn abort(&mut self, err: moq_net::Error) { + /// + /// Consumes the publisher: the broadcast is done. + fn abort(self, err: moq_net::Error) { self.importer.abort(err); } } diff --git a/rs/moq-rtmp/src/server.rs b/rs/moq-rtmp/src/server.rs index 35051cab8f..ca1ec0b659 100644 --- a/rs/moq-rtmp/src/server.rs +++ b/rs/moq-rtmp/src/server.rs @@ -1212,7 +1212,9 @@ impl Publisher { /// Abort the published tracks with `err` so subscribers see the real cause /// (the client disconnected, a protocol error) rather than a generic /// `Error::Dropped` from the importer being dropped. - fn abort(&mut self, err: moq_net::Error) { + /// + /// Consumes the publisher: the broadcast is done. + fn abort(self, err: moq_net::Error) { self.importer.abort(err); } } diff --git a/rs/moq-srt/src/ts.rs b/rs/moq-srt/src/ts.rs index c8194fb538..943f133059 100644 --- a/rs/moq-srt/src/ts.rs +++ b/rs/moq-srt/src/ts.rs @@ -63,7 +63,9 @@ impl Publisher { /// Abort the published tracks with `err` so subscribers see the real cause /// (the SRT caller dropped, a demux error) rather than a generic `Error::Dropped`. - pub fn abort(&mut self, err: moq_net::Error) { + /// + /// Consumes the publisher: the broadcast is done. + pub fn abort(self, err: moq_net::Error) { self.importer.abort(err); } } diff --git a/rs/moq-transcode/src/rung.rs b/rs/moq-transcode/src/rung.rs index ad9f157616..6f28b3388b 100644 --- a/rs/moq-transcode/src/rung.rs +++ b/rs/moq-transcode/src/rung.rs @@ -105,7 +105,7 @@ async fn live(rung: &Rung, producer: &mut moq_net::track::Producer) -> Result<() }, err = rung.broadcast.closed() => { // The source went away while idle; end the rung with it. - producer.abort(err)?; + producer.clone().abort(err)?; return Ok(()); } } @@ -126,7 +126,7 @@ async fn live(rung: &Rung, producer: &mut moq_net::track::Producer) -> Result<() let item = tokio::select! { item = listener.recv() => item, _ = demand.unused() => { - if let Some(mut output) = current.take() { + if let Some(output) = current.take() { // Signal downstream that the group is incomplete. output.abort(moq_net::Error::Cancel)?; } @@ -136,7 +136,7 @@ async fn live(rung: &Rung, producer: &mut moq_net::track::Producer) -> Result<() match item { Some(Item::Group(sequence)) => { - if let Some(mut output) = current.take() { + if let Some(output) = current.take() { // A group boundary without an end: treat as incomplete. output.abort(moq_net::Error::Cancel)?; } @@ -186,13 +186,13 @@ async fn live(rung: &Rung, producer: &mut moq_net::track::Producer) -> Result<() Some(Item::Lagged) => { // Fell behind the feed: abandon the group and resume at the // next boundary rather than stalling other rungs. - if let Some(mut output) = current.take() { + if let Some(output) = current.take() { output.abort(moq_net::Error::Cancel)?; } } Some(Item::Finished) => { // The source track ended: the derivative ends with it. - if let Some(mut output) = current.take() { + if let Some(output) = current.take() { output.abort(moq_net::Error::Cancel)?; } producer.finish()?; @@ -200,10 +200,10 @@ async fn live(rung: &Rung, producer: &mut moq_net::track::Producer) -> Result<() } None => { // The feed died mid-stream (source or decode error). - if let Some(mut output) = current.take() { + if let Some(output) = current.take() { let _ = output.abort(moq_net::Error::Cancel); } - producer.abort(moq_net::Error::Cancel)?; + producer.clone().abort(moq_net::Error::Cancel)?; return Ok(()); } } @@ -278,11 +278,11 @@ async fn fetch(rung: Rung, request: moq_net::track::GroupRequest) -> Result<(), } }; - let mut output = match request.accept(None) { + let output = match request.accept(None) { Ok(output) => output, Err(err) => return Err(err.into()), }; - transcode_group(pipeline, &container, &mut source, &mut output).await?; + transcode_group(pipeline, &container, &mut source, output).await?; Ok(()) } @@ -293,9 +293,9 @@ async fn transcode_group( pipeline: Pipeline, container: &moq_mux::catalog::hang::Container, source: &mut moq_net::group::Consumer, - output: &mut moq_net::group::Producer, + mut output: moq_net::group::Producer, ) -> Result<(), Error> { - match transcode_group_inner(pipeline, container, source, output).await { + match transcode_group_inner(pipeline, container, source, &mut output).await { Ok(()) => { output.finish()?; Ok(()) diff --git a/rs/moq-video/src/encode/producer.rs b/rs/moq-video/src/encode/producer.rs index 196c7db538..f997807d29 100644 --- a/rs/moq-video/src/encode/producer.rs +++ b/rs/moq-video/src/encode/producer.rs @@ -58,14 +58,14 @@ impl Producer { let track = moq_mux::import::unique_track(&mut broadcast, ".avc3")?; Codecs::H264 { split: moq_mux::codec::h264::Split::new(), - import: moq_mux::codec::h264::Import::new(track, catalog.reserve(), Default::default()), + import: moq_mux::codec::h264::Import::new(track, catalog.reserve(), Default::default())?, } } Codec::H265 => { let track = moq_mux::import::unique_track(&mut broadcast, ".hev1")?; Codecs::H265 { split: moq_mux::codec::h265::Split::new(), - import: moq_mux::codec::h265::Import::new(track, catalog.reserve(), Default::default()), + import: moq_mux::codec::h265::Import::new(track, catalog.reserve(), Default::default())?, } } }; @@ -119,8 +119,8 @@ impl Producer { /// see the real cause rather than [`moq_net::Error::Dropped`]. /// /// Consumes the producer, like [`finish`](Self::finish). - pub fn abort(mut self, err: moq_net::Error) { - match &mut self.codecs { + pub fn abort(self, err: moq_net::Error) { + match self.codecs { Codecs::H264 { import, .. } => import.abort(err), Codecs::H265 { import, .. } => import.abort(err), } From 03ec0bd56061d120d2c7d463418419496467630f Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 21 Jul 2026 15:25:50 -0700 Subject: [PATCH 03/11] test(mux): drop clone() workarounds around the borrowing finish `timeline::Producer::finish` and `catalog::Producer::finish` take `&mut self`, so these tests never needed to finish through a throwaway clone. Bind the producer mutably and finish it directly. Leaves the three `broadcast.clone().finish()` sites in moq-rtmp and moq-srt alone. `broadcast::Producer::finish` consumes `self` for a reason: it only sets `closing`, and the broadcast ends when the last producer handle drops, so consuming the handle is the mechanism rather than a signature preference. Relaxing it to `&mut self` marks intent without ever ending the broadcast, which hangs the moq-hls renditions cursor. Co-Authored-By: Claude Opus 4.8 --- rs/moq-mux/src/catalog/producer.rs | 3 +-- rs/moq-mux/src/timeline.rs | 16 ++++++++-------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/rs/moq-mux/src/catalog/producer.rs b/rs/moq-mux/src/catalog/producer.rs index f2855f6730..0adad76901 100644 --- a/rs/moq-mux/src/catalog/producer.rs +++ b/rs/moq-mux/src/catalog/producer.rs @@ -513,8 +513,7 @@ mod test { let mut catalog = Producer::new(&mut broadcast).unwrap(); // Finished tracks can't take another group, so the publish behind the guard fails. - // The producer is a shared handle, so finish a clone and keep this one to publish through. - catalog.clone().finish().unwrap(); + catalog.finish().unwrap(); let mut guard = catalog.lock(); guard diff --git a/rs/moq-mux/src/timeline.rs b/rs/moq-mux/src/timeline.rs index 75bd8c5d9b..23bfa05ee1 100644 --- a/rs/moq-mux/src/timeline.rs +++ b/rs/moq-mux/src/timeline.rs @@ -294,7 +294,7 @@ mod test { #[tokio::test] async fn records_group_opens_in_milliseconds() { let mut broadcast = moq_net::broadcast::Info::new().produce(); - let timeline = Producer::new(&mut broadcast, "video0").unwrap(); + let mut timeline = Producer::new(&mut broadcast, "video0").unwrap(); assert_eq!(timeline.track, "video0.timeline.z"); let track = broadcast.create_track("video0", None).unwrap(); @@ -305,7 +305,7 @@ mod test { media.write(frame(2_000_000, false)).unwrap(); // extends group 0 media.write(frame(4_000_000, true)).unwrap(); // group 1 @ 4_000_000us media.finish().unwrap(); - timeline.clone().finish().unwrap(); + timeline.finish().unwrap(); // Entry pts is a real Timestamp (decoded from the ms-timescale record). assert_eq!(drain(&broadcast, &timeline).await, vec![entry(0, 0), entry(1, 4_000)]); @@ -314,7 +314,7 @@ mod test { #[tokio::test] async fn granularity_throttles_records() { let mut broadcast = moq_net::broadcast::Info::new().produce(); - let timeline = Producer::new(&mut broadcast, "audio0").unwrap(); + let mut timeline = Producer::new(&mut broadcast, "audio0").unwrap(); let mut recorder = timeline.recorder(); // Default granularity is 1s. Group opens 300ms apart, all within a second of the first, then @@ -323,7 +323,7 @@ mod test { recorder.record(seq, Timestamp::from_millis(ms).unwrap()).unwrap(); } drop(recorder); - timeline.clone().finish().unwrap(); + timeline.finish().unwrap(); assert_eq!(drain(&broadcast, &timeline).await, vec![entry(0, 0), entry(4, 1200)]); } @@ -353,8 +353,8 @@ mod test { #[tokio::test] async fn rejects_an_invalid_timescale() { let mut broadcast = moq_net::broadcast::Info::new().produce(); - let timeline = Producer::new(&mut broadcast, "video0").unwrap(); - timeline.clone().finish().unwrap(); + let mut timeline = Producer::new(&mut broadcast, "video0").unwrap(); + timeline.finish().unwrap(); // A timescale of 0 can't be honored, and quietly reading the track at milliseconds would // report timestamps the publisher never meant. @@ -393,12 +393,12 @@ mod test { #[tokio::test] async fn consumer_decodes_pts_from_the_section() { let mut broadcast = moq_net::broadcast::Info::new().produce(); - let timeline = Producer::new(&mut broadcast, "video0").unwrap(); + let mut timeline = Producer::new(&mut broadcast, "video0").unwrap(); timeline .recorder() .record(3, Timestamp::from_micros(7_000).unwrap()) .unwrap(); - timeline.clone().finish().unwrap(); + timeline.finish().unwrap(); // The reader takes the track name + timescale from the section, and yields a real Timestamp. // The pts is decoded at the timeline's (millisecond) timescale, so compare the instant rather From 558df98600f8af2e27175ff3bfe0bceef45594bb Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 21 Jul 2026 15:47:36 -0700 Subject: [PATCH 04/11] fix(net)!: make broadcast::Producer::finish actually end the broadcast `finish` took `self` and only set `closing`, which gates serving new tracks. What consumers actually wait on is the separate `alive` channel, and that closed only once every producer handle dropped. So finishing did not end the broadcast; dropping the last handle did, and consuming `self` was standing in for that. The signature made that unusable for anything holding a producer in a struct, and three gateways worked around it with `self.broadcast.clone().finish()`, which is strictly worse: it sets the flag and drops a throwaway clone while the real handle lives on, so the broadcast keeps lingering. `finish` now takes `&mut self` and closes `alive`, so it ends the broadcast outright whether or not other clones are alive. Existing tracks stay readable, per the no-cascading-close rule. This matches `track::Producer::finish` and `group::Producer::finish`: finishing declares the end, and must not depend on the caller also surrendering the handle. The crate-private `abort(&self)` keeps the old flag-only behavior. It is used by sessions tearing down announced broadcasts, where the broadcast may still linger for a reconnect, so it is a genuinely different operation rather than a `&self` copy of `finish`. Drops the three `clone().finish()` workarounds in moq-rtmp and moq-srt. Co-Authored-By: Claude Opus 4.8 --- rs/libmoq/src/publish.rs | 2 +- rs/moq-hls/src/server/mod.rs | 4 ++-- rs/moq-native/tests/broadcast.rs | 4 ++-- rs/moq-net/src/model/broadcast.rs | 31 +++++++++++++++++----------- rs/moq-net/src/model/origin.rs | 34 +++++++++++++++---------------- rs/moq-relay/src/cluster.rs | 2 +- rs/moq-rtc/src/server/mod.rs | 2 +- rs/moq-rtmp/src/dial.rs | 2 +- rs/moq-rtmp/src/server.rs | 2 +- rs/moq-srt/src/ts.rs | 2 +- rs/moq-stats/src/produce.rs | 4 ++-- 11 files changed, 48 insertions(+), 41 deletions(-) diff --git a/rs/libmoq/src/publish.rs b/rs/libmoq/src/publish.rs index 26ec8d010e..0aed2bfe7e 100644 --- a/rs/libmoq/src/publish.rs +++ b/rs/libmoq/src/publish.rs @@ -73,7 +73,7 @@ impl Publish { /// Cleanly finish the broadcast and finalize the catalog stream, so subscribers /// see a normal end rather than [`moq_net::Error::Dropped`]. pub fn finish(&mut self, broadcast: Id) -> Result<(), Error> { - let (broadcast, mut catalog) = self.broadcasts.remove(broadcast).ok_or(Error::BroadcastNotFound)?; + let (mut broadcast, mut catalog) = self.broadcasts.remove(broadcast).ok_or(Error::BroadcastNotFound)?; // Finish the broadcast first so the clean end reaches subscribers even if // finalizing the catalog fails. broadcast.finish(); diff --git a/rs/moq-hls/src/server/mod.rs b/rs/moq-hls/src/server/mod.rs index 4514a1078b..ffa32217a1 100644 --- a/rs/moq-hls/src/server/mod.rs +++ b/rs/moq-hls/src/server/mod.rs @@ -198,7 +198,7 @@ mod tests { async fn closed_broadcaster() -> Arc { let origin = moq_net::Origin::random().produce(); - let producer = origin + let mut producer = origin .create_broadcast("gone", moq_net::broadcast::Route::new().with_announce(true)) .expect("publish allowed"); settle().await; @@ -240,7 +240,7 @@ mod tests { let origin = moq_net::Origin::random().produce(); let server = Server::new(origin.consume(), Config::default()); let old = closed_broadcaster().await; - let new_producer = origin + let mut new_producer = origin .create_broadcast("live", moq_net::broadcast::Route::new().with_announce(true)) .expect("publish allowed"); settle().await; diff --git a/rs/moq-native/tests/broadcast.rs b/rs/moq-native/tests/broadcast.rs index bfb9778bb1..749f9fa02c 100644 --- a/rs/moq-native/tests/broadcast.rs +++ b/rs/moq-native/tests/broadcast.rs @@ -590,7 +590,7 @@ async fn broadcast_moq_lite_06_announce_lifecycle() { let pub_origin = Origin::random().produce(); // Announced before the client connects, so it rides the initial set. - let first = pub_origin + let mut first = pub_origin .create_broadcast("first", moq_net::broadcast::Route::new().with_announce(true)) .expect("create broadcast"); @@ -630,7 +630,7 @@ async fn broadcast_moq_lite_06_announce_lifecycle() { assert!(broadcast.is_some(), "expected initial announce"); // A live announce after the initial set. - let second = pub_origin + let mut second = pub_origin .create_broadcast("second", moq_net::broadcast::Route::new().with_announce(true)) .expect("create broadcast"); let moq_net::announce::Update { path, broadcast } = next_announce(&mut announcements).await; diff --git a/rs/moq-net/src/model/broadcast.rs b/rs/moq-net/src/model/broadcast.rs index d90f638f89..693211b08e 100644 --- a/rs/moq-net/src/model/broadcast.rs +++ b/rs/moq-net/src/model/broadcast.rs @@ -2,8 +2,8 @@ //! //! A [Producer] creates tracks on demand: a [Consumer] subscribes by name, and the //! producer either serves a track it already has or is handed a [`track::Request`] to -//! fill. Both handles are refcounted clones of one broadcast, which closes when the -//! last producer drops. +//! fill. Both handles are refcounted clones of one broadcast, which closes on +//! [`Producer::finish`] or when the last producer drops. //! //! [Info] is the static metadata; [Route] is the dynamic path the broadcast takes to //! reach an origin, including whether it is announced to subscribers. @@ -445,17 +445,24 @@ impl Producer { /// end. Prefer this over dropping the producer: an accidental drop (see the note /// on [`Producer`]) logs a warning, whereas `finish()` is silent. /// - /// Only marks intent; the broadcast actually ends once every producer clone is - /// gone, so a clone that outlives this call keeps it alive until it too is - /// dropped or finished. - pub fn finish(self) { + /// Ends the broadcast outright: consumers observe a normal end immediately and no + /// new tracks are served, whether or not other producer clones are still alive. + /// Existing tracks stay readable so consumers can drain what they already have. + /// + /// Borrows rather than consumes, matching [`track::Producer::finish`]. Finishing + /// declares the end, so it must not depend on the caller also surrendering the + /// handle. + pub fn finish(&mut self) { self.state.lock().closing = true; + // Ending the broadcast is what consumers wait on, so signal it here rather + // than leaving it to the last handle drop. + let _ = self.alive.close(); } - /// Mark the broadcast as deliberately ended, without the - /// dropped-without-finish warning. Same effect as [`Self::finish`], but takes - /// `&self` for callers that can't consume the producer. Used by sessions - /// tearing down announced broadcasts when the connection dies. + /// Mark the broadcast as deliberately ended so the drop path doesn't warn, without + /// ending it for consumers the way [`Self::finish`] does. Used by sessions tearing + /// down announced broadcasts when the connection dies, where the broadcast may + /// still linger for a reconnect. pub(crate) fn abort(&self) { self.state.lock().closing = true; } @@ -522,7 +529,7 @@ impl SourceGuard { /// End the source deliberately: the origin detaches it immediately, /// unannouncing the path if it was the last. pub fn finish(mut self) { - if let Some(producer) = self.producer.take() { + if let Some(mut producer) = self.producer.take() { producer.finish(); } } @@ -642,7 +649,7 @@ impl Dynamic { } /// Poll until the broadcast closes; ready with the cause (always [`Error::Dropped`], - /// since a broadcast only ends by every producer dropping). + /// whether it ended via [`Producer::finish`] or by every producer dropping). pub fn poll_closed(&self, waiter: &kio::Waiter) -> Poll { self.alive.poll_closed(waiter).map(|()| Error::Dropped) } diff --git a/rs/moq-net/src/model/origin.rs b/rs/moq-net/src/model/origin.rs index d2b21c50bd..f20de317ea 100644 --- a/rs/moq-net/src/model/origin.rs +++ b/rs/moq-net/src/model/origin.rs @@ -1297,7 +1297,7 @@ fn attach_source( /// until the last source detaches, then unpublishes the broadcast. async fn run_front( state: kio::Producer, - broadcast: broadcast::Producer, + mut broadcast: broadcast::Producer, node: Lock, rest: PathOwned, ) { @@ -2777,7 +2777,7 @@ mod tests { consumer1.assert_next_wait(); // Publish the first broadcast; it becomes visible asynchronously. - let broadcast1 = origin.create_broadcast("test1", announce()).unwrap(); + let mut broadcast1 = origin.create_broadcast("test1", announce()).unwrap(); settle().await; consumer1.assert_next_some("test1"); @@ -2788,7 +2788,7 @@ mod tests { let mut consumer2 = origin.consume().announced(); // Publish the second broadcast. - let broadcast2 = origin.create_broadcast("test2", announce()).unwrap(); + let mut broadcast2 = origin.create_broadcast("test2", announce()).unwrap(); settle().await; consumer1.assert_next_some("test2"); @@ -2832,9 +2832,9 @@ mod tests { let consumer = origin.consume(); let mut announced = consumer.announced(); - let broadcast1 = origin.create_broadcast("test", announce()).unwrap(); - let broadcast2 = origin.create_broadcast("test", announce()).unwrap(); - let broadcast3 = origin.create_broadcast("test", announce()).unwrap(); + let mut broadcast1 = origin.create_broadcast("test", announce()).unwrap(); + let mut broadcast2 = origin.create_broadcast("test", announce()).unwrap(); + let mut broadcast3 = origin.create_broadcast("test", announce()).unwrap(); settle().await; assert!(consumer.get_broadcast("test").is_some()); @@ -3187,7 +3187,7 @@ mod tests { let mut announced = consumer.announced(); let hops = OriginList::try_from(vec![Origin::new(1).unwrap()]).unwrap(); - let source = origin + let mut source = origin .create_broadcast("test", announce().with_hops(hops.clone())) .unwrap(); settle().await; @@ -3323,7 +3323,7 @@ mod tests { // An announced source with a worse cost still wins: the path announces // and advertises its route. - let announced_source = origin.create_broadcast("test", announce().with_cost(10)).unwrap(); + let mut announced_source = origin.create_broadcast("test", announce().with_cost(10)).unwrap(); settle().await; announced.assert_next_some("test"); let face = consumer.get_broadcast("test").unwrap(); @@ -3368,8 +3368,8 @@ mod tests { let origin = Origin::random().produce(); - let broadcast1 = origin.create_broadcast("test", announce()).unwrap(); - let broadcast2 = origin.create_broadcast("test", announce()).unwrap(); + let mut broadcast1 = origin.create_broadcast("test", announce()).unwrap(); + let mut broadcast2 = origin.create_broadcast("test", announce()).unwrap(); settle().await; assert!(origin.consume().get_broadcast("test").is_some()); @@ -4060,7 +4060,7 @@ mod tests { let prefix = "some_prefix/".to_string(); let mut consumer = origin.consume().with_root(prefix).unwrap().announced(); - let b = origin.create_broadcast("some_prefix/test", announce()).unwrap(); + let mut b = origin.create_broadcast("some_prefix/test", announce()).unwrap(); settle().await; consumer.assert_next_some("test"); @@ -4320,7 +4320,7 @@ mod tests { let origin = Origin::random().produce(); let mut announced = origin.consume().announced(); - let broadcast = origin.create_broadcast("test", announce()).unwrap(); + let mut broadcast = origin.create_broadcast("test", announce()).unwrap(); settle().await; broadcast.finish(); @@ -4338,7 +4338,7 @@ mod tests { let origin = Origin::random().produce(); let mut announced = origin.consume().announced(); - let broadcast1 = origin.create_broadcast("test", announce()).unwrap(); + let mut broadcast1 = origin.create_broadcast("test", announce()).unwrap(); settle().await; broadcast1.finish(); settle().await; @@ -4356,7 +4356,7 @@ mod tests { tokio::time::pause(); let origin = Origin::random().produce(); - let broadcast1 = origin.create_broadcast("test", announce()).unwrap(); + let mut broadcast1 = origin.create_broadcast("test", announce()).unwrap(); settle().await; let mut announced = origin.consume().announced(); @@ -4382,7 +4382,7 @@ mod tests { tokio::time::pause(); let origin = Origin::random().produce(); - let broadcast1 = origin.create_broadcast("test", announce()).unwrap(); + let mut broadcast1 = origin.create_broadcast("test", announce()).unwrap(); settle().await; let mut announced = origin.consume().announced(); @@ -4391,7 +4391,7 @@ mod tests { broadcast1.finish(); settle().await; - let broadcast2 = origin.create_broadcast("test", announce()).unwrap(); + let mut broadcast2 = origin.create_broadcast("test", announce()).unwrap(); settle().await; broadcast2.finish(); settle().await; @@ -4412,7 +4412,7 @@ mod tests { let mut announced = origin.consume().announced(); for _ in 0..1000 { - let broadcast = origin.create_broadcast("test", announce()).unwrap(); + let mut broadcast = origin.create_broadcast("test", announce()).unwrap(); settle().await; broadcast.finish(); } diff --git a/rs/moq-relay/src/cluster.rs b/rs/moq-relay/src/cluster.rs index 803c389126..f83a57164d 100644 --- a/rs/moq-relay/src/cluster.rs +++ b/rs/moq-relay/src/cluster.rs @@ -612,7 +612,7 @@ impl Cluster { // Deliberate shutdown: finish the registration rather than dropping it, so // there is no dropped-without-finish warning. - if let Some(registration) = self_registration { + if let Some(mut registration) = self_registration { registration.finish(); } Ok(()) diff --git a/rs/moq-rtc/src/server/mod.rs b/rs/moq-rtc/src/server/mod.rs index a94d5b65f9..56db5fa776 100644 --- a/rs/moq-rtc/src/server/mod.rs +++ b/rs/moq-rtc/src/server/mod.rs @@ -88,7 +88,7 @@ impl AcceptedSession { tracing::debug!(role = self.role, "webrtc session terminated by DELETE"); // A deliberate end: finish the broadcast so the origin // unannounces it immediately. - if let Some(broadcast) = self.broadcast.take() { + if let Some(mut broadcast) = self.broadcast.take() { broadcast.finish(); } Ok(()) diff --git a/rs/moq-rtmp/src/dial.rs b/rs/moq-rtmp/src/dial.rs index f7bca26164..df7c253e8a 100644 --- a/rs/moq-rtmp/src/dial.rs +++ b/rs/moq-rtmp/src/dial.rs @@ -475,7 +475,7 @@ impl Publisher { fn finish(&mut self) -> anyhow::Result<()> { self.importer.finish()?; - self.broadcast.clone().finish(); + self.broadcast.finish(); Ok(()) } diff --git a/rs/moq-rtmp/src/server.rs b/rs/moq-rtmp/src/server.rs index ca1ec0b659..41b3ff44ea 100644 --- a/rs/moq-rtmp/src/server.rs +++ b/rs/moq-rtmp/src/server.rs @@ -1205,7 +1205,7 @@ impl Publisher { /// the broadcast so the origin unannounces it immediately. fn finish(&mut self) -> anyhow::Result<()> { self.importer.finish()?; - self.broadcast.clone().finish(); + self.broadcast.finish(); Ok(()) } diff --git a/rs/moq-srt/src/ts.rs b/rs/moq-srt/src/ts.rs index 943f133059..8002cafd3b 100644 --- a/rs/moq-srt/src/ts.rs +++ b/rs/moq-srt/src/ts.rs @@ -57,7 +57,7 @@ impl Publisher { /// the broadcast so the origin unannounces it immediately. pub fn finish(&mut self) -> Result<()> { self.importer.finish()?; - self.broadcast.clone().finish(); + self.broadcast.finish(); Ok(()) } diff --git a/rs/moq-stats/src/produce.rs b/rs/moq-stats/src/produce.rs index fc138a2724..6b47b16f1b 100644 --- a/rs/moq-stats/src/produce.rs +++ b/rs/moq-stats/src/produce.rs @@ -205,7 +205,7 @@ impl Task { ticker.tick().await; if weak.upgrade().is_none() { - for (_, publisher) in groups.drain() { + for (_, mut publisher) in groups.drain() { publisher.broadcast.finish(); } return; @@ -318,7 +318,7 @@ impl Task { .cloned() .collect(); for group in evicted { - if let Some(publisher) = groups.remove(&group) { + if let Some(mut publisher) = groups.remove(&group) { publisher.broadcast.finish(); } } From c7d1086824f3d67c111729cc20db48bad9a10b7a Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 21 Jul 2026 16:02:18 -0700 Subject: [PATCH 05/11] fix(mux,rtc): don't advertise renditions a fallible producer never backed Making `catalog::Producer::media_producer` return `Result` earlier in this branch turned three previously-safe orderings into stranded-rendition bugs. Found by CodeRabbit on the PR. `mkv` and `ts` import both published a rendition into the catalog and only then built its media producer. Now that the producer can fail (it mints the rendition's `.timeline.z` track, which can collide) the catalog is left advertising a rendition nothing serves. `ts` is the worse of the two: its `VerbatimEntry` guard, which removes the entry on drop, is only constructed once the function returns successfully, so nothing cleans up. Both now build the producer first and publish only once it exists. The moq-rtc VP8 and VP9 bridges mutated the catalog through a temporary `lock()` guard, whose drop publishes and merely warns on failure, and then latched `announced = true` unconditionally. A closed catalog therefore returned `Ok(())` while leaving the media track advertised nowhere, and `announced` latches so it would never retry. They now commit explicitly and only latch after the commit succeeds, which is what `Guard::commit` was added for. Regression test on the mkv path: it squats the timeline track name so the producer fails, and asserts the rendition is absent. Verified to fail against the previous ordering. Also documents `loc` in the native container list, which listed only legacy and cmaf and so implied `loc` decodes as `Unknown`, and corrects a test comment that claimed `Import::finish` drops the importer (it borrows). Co-Authored-By: Claude Opus 4.8 --- doc/lib/rs/env/native.md | 1 + rs/moq-mux/src/container/mkv/import.rs | 17 +++++++---- rs/moq-mux/src/container/mkv/import_test.rs | 33 +++++++++++++++++++++ rs/moq-mux/src/container/ts/import.rs | 15 +++++++--- rs/moq-rtc/src/codec/vp8.rs | 7 ++++- rs/moq-rtc/src/codec/vp9.rs | 7 ++++- 6 files changed, 69 insertions(+), 11 deletions(-) diff --git a/doc/lib/rs/env/native.md b/doc/lib/rs/env/native.md index 4cf8381070..074977007b 100644 --- a/doc/lib/rs/env/native.md +++ b/doc/lib/rs/env/native.md @@ -185,6 +185,7 @@ Check the `container` field for each rendition: - **`legacy`** — Each frame is a varint timestamp (microseconds) followed by the codec payload. This is the common case. - **`cmaf`** — Each frame is a `moof` + `mdat` pair (fragmented MP4). Used for HLS compatibility. +- **`loc`** — Low Overhead Container: each frame is a small property block followed by the codec payload. `OrderedConsumer` decodes legacy timestamps for you automatically. diff --git a/rs/moq-mux/src/container/mkv/import.rs b/rs/moq-mux/src/container/mkv/import.rs index b3c820c15c..eb8436d318 100644 --- a/rs/moq-mux/src/container/mkv/import.rs +++ b/rs/moq-mux/src/container/mkv/import.rs @@ -273,17 +273,26 @@ impl Import { let track = self .broadcast .create_track(self.broadcast.unique_name(suffix), hang::container::track_info())?; + let name = track.name().to_string(); + + // Build the media producer before publishing the rendition. It is fallible (its + // timeline track can collide), and a rendition published for a track we then fail + // to produce would be advertised to consumers but never served. + let media = self + .catalog + .media_producer(track, crate::catalog::hang::Container::Legacy)?; + let mut catalog = self.catalog.clone(); let mut catalog = catalog.lock(); match kind { TrackKind::Video => { let config = build_video_config(&codec_id, codec_private.as_ref(), video_children.as_deref())?; - catalog.video.renditions.insert(track.name().to_string(), config); + catalog.video.renditions.insert(name, config); } TrackKind::Audio => { let config = build_audio_config(&codec_id, codec_private.as_ref(), audio_children.as_deref())?; - catalog.audio.renditions.insert(track.name().to_string(), config); + catalog.audio.renditions.insert(name, config); } } @@ -293,9 +302,7 @@ impl Import { track_number, MkvTrack { kind, - track: self - .catalog - .media_producer(track, crate::catalog::hang::Container::Legacy)?, + track: media, group: None, last_emitted_ticks: None, }, diff --git a/rs/moq-mux/src/container/mkv/import_test.rs b/rs/moq-mux/src/container/mkv/import_test.rs index a3f94aa0f4..e121b4c2d7 100644 --- a/rs/moq-mux/src/container/mkv/import_test.rs +++ b/rs/moq-mux/src/container/mkv/import_test.rs @@ -373,3 +373,36 @@ fn test_block_timestamp_scaling() { // rendition wiring. let _ = run(&data); } + +/// A rendition must never be advertised when its media producer could not be built. +/// +/// `media_producer` is fallible (it mints the rendition's `.timeline.z` track, which can +/// collide), so publishing the catalog entry first would leave consumers a rendition that is +/// announced but has no producer behind it and is therefore never served. +#[test] +fn rendition_is_not_published_when_the_media_producer_fails() { + let data = MkvBuilder::new() + .header("webm") + .segment_start() + .info(1_000_000) + .tracks(vec![track_entry_video_vp9(1, 640, 480)]) + .segment_end() + .build(); + + let mut broadcast = moq_net::broadcast::Info::new().produce(); + let catalog = crate::catalog::Producer::new(&mut broadcast).unwrap(); + + // Squat the timeline track the first video rendition will want, so building its media + // producer fails. `unique_name` is deterministic, so this is the name it will pick. The + // handle must stay alive: the broadcast tracks names weakly, so dropping it frees the name. + let _squat = broadcast.create_track("0.mkv-v.timeline.z", None).unwrap(); + + let mut mkv = crate::container::mkv::Import::new(broadcast, catalog.reserve()); + let buf = bytes::BytesMut::from(&data[..]); + let _ = mkv.decode(&buf); + + assert!( + catalog.snapshot().video.renditions.is_empty(), + "a rendition whose media producer failed must not be advertised" + ); +} diff --git a/rs/moq-mux/src/container/ts/import.rs b/rs/moq-mux/src/container/ts/import.rs index 732acdc0d2..12e1259167 100644 --- a/rs/moq-mux/src/container/ts/import.rs +++ b/rs/moq-mux/src/container/ts/import.rs @@ -644,6 +644,13 @@ fn register_verbatim( // timestamp to microseconds on the wire (see `hang::container::Frame::encode`), // so the track declares that timescale to match. let track = broadcast.unique_track(".ts", hang::container::track_info())?; + let name = track.name().to_string(); + + // Build the media producer before advertising the track. It is fallible (its + // timeline track can collide), and the `VerbatimEntry` that removes this catalog + // entry on drop only exists once this function returns successfully, so an entry + // published first would be stranded. + let media = catalog.media_producer(track, crate::catalog::hang::Container::Legacy)?; let mut guard = catalog.lock(); let Some(mpegts) = guard.mpegts_mut() else { @@ -652,7 +659,7 @@ fn register_verbatim( anyhow::bail!("catalog extension no longer carries an mpegts section"); }; mpegts.tracks.insert( - track.name().to_string(), + name, catalog::Track { pid, descriptors, @@ -661,7 +668,7 @@ fn register_verbatim( ); drop(guard); - Ok(catalog.media_producer(track, crate::catalog::hang::Container::Legacy)?) + Ok(media) } /// Remove a verbatim track's entry from the `mpegts` catalog section on drop. @@ -1940,8 +1947,8 @@ mod test { "upgrade advertises the cue track" ); - // Finishing drops the importer, which clears its verbatim entries from the catalog, so - // read the track name first. + // The importer clears its verbatim entries from the catalog when it drops, so read the + // track name while it is still registered. let name = catalog.snapshot().mpegts.tracks.keys().next().unwrap().clone(); import.finish().unwrap(); let track = consumer.track(&name).unwrap().subscribe(None).await.unwrap(); diff --git a/rs/moq-rtc/src/codec/vp8.rs b/rs/moq-rtc/src/codec/vp8.rs index 1c582eded4..d2008bf2f4 100644 --- a/rs/moq-rtc/src/codec/vp8.rs +++ b/rs/moq-rtc/src/codec/vp8.rs @@ -35,7 +35,12 @@ impl Bridge { let mut config = hang::catalog::VideoConfig::new(hang::catalog::VideoCodec::VP8); config.container = hang::catalog::Container::Legacy; config.timeline = Some(self.rendition.catalog.timeline(&name)?.section()); - self.rendition.catalog.lock().video.renditions.insert(name, config); + // Publish explicitly rather than through the guard's drop, which only warns: + // marking the rendition announced when the catalog never took it would leave the + // media track advertised nowhere, and `announced` latches so we'd never retry. + let mut guard = self.rendition.catalog.lock(); + guard.video.renditions.insert(name, config); + guard.commit()?; self.announced = true; Ok(()) } diff --git a/rs/moq-rtc/src/codec/vp9.rs b/rs/moq-rtc/src/codec/vp9.rs index cc275bfbfe..7560220c34 100644 --- a/rs/moq-rtc/src/codec/vp9.rs +++ b/rs/moq-rtc/src/codec/vp9.rs @@ -34,7 +34,12 @@ impl Bridge { let mut config = hang::catalog::VideoConfig::new(hang::catalog::VP9::default()); config.container = hang::catalog::Container::Legacy; config.timeline = Some(self.rendition.catalog.timeline(&name)?.section()); - self.rendition.catalog.lock().video.renditions.insert(name, config); + // Publish explicitly rather than through the guard's drop, which only warns: + // marking the rendition announced when the catalog never took it would leave the + // media track advertised nowhere, and `announced` latches so we'd never retry. + let mut guard = self.rendition.catalog.lock(); + guard.video.renditions.insert(name, config); + guard.commit()?; self.announced = true; Ok(()) } From 02646e570f354c469f1c6c125ed7bc4939ca375b Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Tue, 21 Jul 2026 16:09:44 -0700 Subject: [PATCH 06/11] fix(hang): reject malformed known containers in the JS schema `ContainerSchema` was `z.union([discriminatedUnion, UnknownContainerSchema])`, and zod tries union members in order. A payload like `{"kind":"cmaf"}` with no `init` failed the strict CMAF arm and was then happily accepted by the passthrough arm, which only required `kind: string`. That is worse than being ignored. `containerSupported` and `isCmafContainer` both key on the kind string, so the rendition still reported as decodable CMAF, and the watch decoders went on to call `base64ToBytes(container.init)` on an undefined init segment. The passthrough arm now rejects recognized kinds, so a known kind can only ever parse through its own strict schema and a malformed one fails the whole union. That restores parity with the Rust side, which inspects `kind` first and only routes recognized kinds through strict decoding. Regression test verified to fail against the previous schema. Also makes `moq_token::Jwk` `#[non_exhaustive]` with a `Jwk::new` constructor. Its own docs invite building it by struct literal, so a future JWK parameter would otherwise be a breaking change for external callers. `generate` now builds through the constructor. Both found by CodeRabbit on the PR. Co-Authored-By: Claude Opus 4.8 --- js/hang/src/catalog/container.test.ts | 9 +++++++++ js/hang/src/catalog/container.ts | 10 +++++++++- rs/moq-token/src/generate.rs | 13 ++++--------- rs/moq-token/src/key.rs | 17 +++++++++++++++++ 4 files changed, 39 insertions(+), 10 deletions(-) diff --git a/js/hang/src/catalog/container.test.ts b/js/hang/src/catalog/container.test.ts index 84124d9c19..6c41376b92 100644 --- a/js/hang/src/catalog/container.test.ts +++ b/js/hang/src/catalog/container.test.ts @@ -45,3 +45,12 @@ test("catalog with an unknown container keeps its other renditions", () => { // The unknown rendition survives a republish intact. expect(parsed.video.renditions.future?.container).toEqual({ kind: "future", magic: 7 }); }); + +test("a malformed known container errors instead of degrading to passthrough", () => { + // `cmaf` without `init` fails its own schema. It must NOT fall through to the passthrough + // arm, which would still report kind "cmaf" and hand decoders an undefined init segment. + expect(() => ContainerSchema.parse({ kind: "cmaf" })).toThrow(); + + // A genuinely unrecognized kind still parses, so one future rendition can't fail the catalog. + expect(ContainerSchema.parse({ kind: "future", magic: 7 })).toEqual({ kind: "future", magic: 7 }); +}); diff --git a/js/hang/src/catalog/container.ts b/js/hang/src/catalog/container.ts index 05f7886d15..ce2abd6096 100644 --- a/js/hang/src/catalog/container.ts +++ b/js/hang/src/catalog/container.ts @@ -8,9 +8,17 @@ const KNOWN_KINDS = ["legacy", "cmaf", "loc"]; * * Kept intact so reparsing and republishing a catalog round-trips the rendition instead of * corrupting it. Such a rendition must be ignored rather than decoded. + * + * Recognized kinds are rejected here so they can only ever parse through their own strict + * schema. Without that, a malformed known container (`{"kind":"cmaf"}` with no `init`) would + * fall through to this arm, still report as CMAF, and hand decoders an undefined init segment. */ export const UnknownContainerSchema = z.looseObject({ - kind: z.string(), + kind: z.string().check( + z.refine((kind) => !KNOWN_KINDS.includes(kind), { + message: "recognized container kind must match its own schema", + }), + ), }); /** diff --git a/rs/moq-token/src/generate.rs b/rs/moq-token/src/generate.rs index a7d18aa51a..27e3cdcc83 100644 --- a/rs/moq-token/src/generate.rs +++ b/rs/moq-token/src/generate.rs @@ -1,5 +1,5 @@ use crate::error::KeyError; -use crate::{Algorithm, EllipticCurve, Jwk, Key, KeyOperation, KeyType, RsaPublicKey}; +use crate::{Algorithm, EllipticCurve, Jwk, Key, KeyType, RsaPublicKey}; use aws_lc_rs::encoding::AsBigEndian; use aws_lc_rs::signature::KeyPair; use p256::elliptic_curve::array::typenum::Unsigned; @@ -21,14 +21,9 @@ pub fn generate(algorithm: Algorithm, id: Option) -> crate::Result Algorithm::EdDSA => generate_ed25519_key(), }; - Jwk { - kid: id, - operations: [KeyOperation::Sign, KeyOperation::Verify].into(), - algorithm, - key: key?, - scope: None, - } - .try_into() + let mut jwk = Jwk::new(algorithm, key?); + jwk.kid = id; + jwk.try_into() } fn generate_hmac_key() -> crate::Result { diff --git a/rs/moq-token/src/key.rs b/rs/moq-token/src/key.rs index 2d77370ab0..caa12cfa98 100644 --- a/rs/moq-token/src/key.rs +++ b/rs/moq-token/src/key.rs @@ -146,6 +146,7 @@ pub struct RsaAdditionalPrime { /// usable on its own: convert it into a [`Key`] via `Key::try_from` to sign or verify anything. #[derive(Clone, Serialize, Deserialize)] #[serde(remote = "Self")] +#[non_exhaustive] pub struct Jwk { /// The algorithm used by the key. #[serde(rename = "alg")] @@ -168,6 +169,22 @@ pub struct Jwk { pub scope: Option, } +impl Jwk { + /// A key that can both sign and verify, with no key ID or scope. + /// + /// Set the remaining fields on the returned value. The struct is `#[non_exhaustive]`, so + /// building it this way keeps working as JWK parameters are added. + pub fn new(algorithm: Algorithm, key: KeyType) -> Self { + Self { + algorithm, + operations: [KeyOperation::Sign, KeyOperation::Verify].into(), + key, + kid: None, + scope: None, + } + } +} + impl<'de> Deserialize<'de> for Jwk { fn deserialize(deserializer: D) -> Result where From c13c1eb6ecf840b8cd64d12c4e74f1a3a98b470c Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 22 Jul 2026 08:42:17 -0700 Subject: [PATCH 07/11] refactor(token,hang)!: restore field access and TypeScript narrowing Two ergonomic regressions from the earlier API fixes, both avoidable without giving up the invariant each one was protecting. `moq_token::Key` now wraps its `Jwk` and derefs to it, so `key.algorithm`, `key.kid`, `key.scope`, `key.operations` and `key.key` read exactly as they did before this branch. There is deliberately no `DerefMut`: handing out `&mut Jwk` would let a caller change the algorithm or key material behind the cached crypto material, which is the whole point of the split. Only construction and mutation break now, which is the part that has to. This follows the existing idiom in moq-net, where `group::Producer` derefs to `group::Info`. Drops the five accessors added earlier, since they duplicate the fields. `hang::catalog::Container` in JS was widened to `kind: string` by the passthrough arm, which broke `kind === "cmaf"` narrowing and forced an `isCmafContainer` guard. The passthrough now maps to a literal tag, `{ kind: "unknown", raw }`, so `Container` is a proper discriminated union again and tolerating a future container costs no type safety. `raw` keeps the original object, including its real `kind`. The type guard is gone; the watch decoders narrow directly. `containerSupported` is now just `kind !== "unknown"`. Nothing on the JS publish path parses and republishes a catalog, so the decode side transform has no wire cost. If that changes, unknown containers need to serialize from `raw` rather than the tagged shape. Co-Authored-By: Claude Opus 4.8 --- js/hang/src/catalog/container.test.ts | 20 +- js/hang/src/catalog/container.ts | 40 ++-- js/watch/src/audio/decoder.ts | 4 +- js/watch/src/video/decoder.ts | 4 +- rs/moq-token/src/key.rs | 255 ++++++++++++-------------- rs/moq-token/src/set.rs | 23 +-- 6 files changed, 164 insertions(+), 182 deletions(-) diff --git a/js/hang/src/catalog/container.test.ts b/js/hang/src/catalog/container.test.ts index 6c41376b92..c3973b7e0e 100644 --- a/js/hang/src/catalog/container.test.ts +++ b/js/hang/src/catalog/container.test.ts @@ -1,9 +1,10 @@ import { expect, test } from "bun:test"; -import { ContainerSchema, containerSupported } from "./container.ts"; +import { type Container, ContainerSchema, containerSupported } from "./container.ts"; import { RootSchema } from "./root.ts"; test("known containers round-trip", () => { - for (const container of [{ kind: "legacy" }, { kind: "cmaf", init: "AAEC" }, { kind: "loc" }]) { + const known: Container[] = [{ kind: "legacy" }, { kind: "cmaf", init: "AAEC" }, { kind: "loc" }]; + for (const container of known) { const parsed = ContainerSchema.parse(container); expect(parsed).toEqual(container); expect(containerSupported(parsed)).toBe(true); @@ -13,7 +14,8 @@ test("known containers round-trip", () => { test("unknown container is preserved instead of throwing", () => { const container = { kind: "future", extra: { nested: [1, 2] }, flag: true }; const parsed = ContainerSchema.parse(container); - expect(parsed).toEqual(container); + // Tagged with a literal `kind` so the union stays discriminated; `raw` is the original. + expect(parsed).toEqual({ kind: "unknown", raw: container }); expect(containerSupported(parsed)).toBe(false); }); @@ -42,8 +44,11 @@ test("catalog with an unknown container keeps its other renditions", () => { expect(known?.container.kind).toBe("legacy"); expect(Number(known?.codedWidth)).toBe(1280); - // The unknown rendition survives a republish intact. - expect(parsed.video.renditions.future?.container).toEqual({ kind: "future", magic: 7 }); + // The unknown rendition keeps its original JSON verbatim under `raw`. + expect(parsed.video.renditions.future?.container).toEqual({ + kind: "unknown", + raw: { kind: "future", magic: 7 }, + }); }); test("a malformed known container errors instead of degrading to passthrough", () => { @@ -52,5 +57,8 @@ test("a malformed known container errors instead of degrading to passthrough", ( expect(() => ContainerSchema.parse({ kind: "cmaf" })).toThrow(); // A genuinely unrecognized kind still parses, so one future rendition can't fail the catalog. - expect(ContainerSchema.parse({ kind: "future", magic: 7 })).toEqual({ kind: "future", magic: 7 }); + expect(ContainerSchema.parse({ kind: "future", magic: 7 })).toEqual({ + kind: "unknown", + raw: { kind: "future", magic: 7 }, + }); }); diff --git a/js/hang/src/catalog/container.ts b/js/hang/src/catalog/container.ts index ce2abd6096..2ab0493bea 100644 --- a/js/hang/src/catalog/container.ts +++ b/js/hang/src/catalog/container.ts @@ -13,13 +13,19 @@ const KNOWN_KINDS = ["legacy", "cmaf", "loc"]; * schema. Without that, a malformed known container (`{"kind":"cmaf"}` with no `init`) would * fall through to this arm, still report as CMAF, and hand decoders an undefined init segment. */ -export const UnknownContainerSchema = z.looseObject({ - kind: z.string().check( - z.refine((kind) => !KNOWN_KINDS.includes(kind), { - message: "recognized container kind must match its own schema", - }), - ), -}); +export const UnknownContainerSchema = z.pipe( + z.looseObject({ + kind: z.string().check( + z.refine((kind) => !KNOWN_KINDS.includes(kind), { + message: "recognized container kind must match its own schema", + }), + ), + }), + // Map to a literal `kind` so {@link Container} stays a discriminated union: a bare + // `kind: string` arm would widen the discriminant and stop `kind === "cmaf"` from + // narrowing. `raw` keeps the original object, including its real `kind`. + z.transform((raw) => ({ kind: "unknown" as const, raw })), +); /** * Container format for frame timestamp encoding and frame payload structure. @@ -52,23 +58,19 @@ export const ContainerSchema = z._default( { kind: "legacy" }, ); -/** The per-frame container format declared in the catalog. */ +/** + * The per-frame container format declared in the catalog. + * + * A discriminated union: `container.kind === "cmaf"` narrows and gives you `init`. An + * unrecognized container arrives as `{ kind: "unknown", raw }` rather than widening `kind`, + * so tolerating a future container costs no type safety here. + */ export type Container = z.infer; /** The CMAF variant of {@link Container}, carrying the base64 init segment. */ export type CmafContainer = Extract; -/** - * Whether the container is CMAF, narrowing it so `init` is available. - * - * The passthrough case makes `kind` a plain string, so an equality check alone no longer - * narrows the union. - */ -export function isCmafContainer(container: Container): container is CmafContainer { - return container.kind === "cmaf"; -} - /** Whether a container can be decoded by this build, i.e. its `kind` is recognized. */ export function containerSupported(container: Container): boolean { - return KNOWN_KINDS.includes(container.kind); + return container.kind !== "unknown"; } diff --git a/js/watch/src/audio/decoder.ts b/js/watch/src/audio/decoder.ts index e8a53d9c50..d8560261cd 100644 --- a/js/watch/src/audio/decoder.ts +++ b/js/watch/src/audio/decoder.ts @@ -332,7 +332,7 @@ export class Decoder { } #runCmafDecoder(effect: Effect, sub: Moq.Track.Subscriber, config: Catalog.AudioConfig): void { - if (!Catalog.isCmafContainer(config.container)) return; // just to help typescript + if (config.container.kind !== "cmaf") return; // just to help typescript const initSegment = base64ToBytes(config.container.init); const init = Container.Cmaf.decodeInitSegment(initSegment); @@ -530,7 +530,7 @@ async function supported(config: Catalog.AudioConfig): Promise { if (config.codec !== "opus") { if (config.description) { description = Util.Hex.toBytes(config.description); - } else if (Catalog.isCmafContainer(config.container)) { + } else if (config.container.kind === "cmaf") { try { description = Container.Cmaf.decodeInitSegment(base64ToBytes(config.container.init)).description; } catch (err) { diff --git a/js/watch/src/video/decoder.ts b/js/watch/src/video/decoder.ts index ef7f8eed82..09bf6cc375 100644 --- a/js/watch/src/video/decoder.ts +++ b/js/watch/src/video/decoder.ts @@ -414,7 +414,7 @@ class DecoderTrack { #runCmaf(effect: Effect, sub: Moq.Track.Subscriber, decoder: VideoDecoder): void { const container = this.config.container; - if (!Catalog.isCmafContainer(container)) return; + if (container.kind !== "cmaf") return; const initSegment = base64ToBytes(container.init); const init = Container.Cmaf.decodeInitSegment(initSegment); @@ -564,7 +564,7 @@ async function supported(config: Catalog.VideoConfig): Promise { let description: Uint8Array | undefined; if (config.description) { description = Util.Hex.toBytes(config.description); - } else if (Catalog.isCmafContainer(config.container)) { + } else if (config.container.kind === "cmaf") { try { description = Container.Cmaf.decodeInitSegment(base64ToBytes(config.container.init)).description; } catch (err) { diff --git a/rs/moq-token/src/key.rs b/rs/moq-token/src/key.rs index caa12cfa98..8dc21da5a8 100644 --- a/rs/moq-token/src/key.rs +++ b/rs/moq-token/src/key.rs @@ -222,17 +222,25 @@ impl Serialize for Jwk { /// key rather than editing an existing one. #[derive(Clone)] pub struct Key { - algorithm: Algorithm, - operations: HashSet, - key: KeyType, - kid: Option, - scope: Option, + jwk: Jwk, // Cached for performance reasons, unfortunately. decode: OnceLock, encode: OnceLock, } +/// Read-only access to the underlying [`Jwk`] fields (`key.algorithm`, `key.kid`, ...). +/// +/// Deliberately no `DerefMut`: handing out `&mut Jwk` would let a caller change the algorithm or +/// key material behind the cached crypto material, which is the bug this split exists to prevent. +impl std::ops::Deref for Key { + type Target = Jwk; + + fn deref(&self) -> &Self::Target { + &self.jwk + } +} + impl TryFrom for Key { type Error = crate::Error; @@ -242,11 +250,7 @@ impl TryFrom for Key { } Ok(Self { - algorithm: jwk.algorithm, - operations: jwk.operations, - key: jwk.key, - kid: jwk.kid, - scope: jwk.scope, + jwk, decode: Default::default(), encode: Default::default(), }) @@ -255,13 +259,7 @@ impl TryFrom for Key { impl From<&Key> for Jwk { fn from(key: &Key) -> Self { - Self { - algorithm: key.algorithm, - operations: key.operations.clone(), - key: key.key.clone(), - kid: key.kid.clone(), - scope: key.scope.clone(), - } + key.jwk.clone() } } @@ -298,31 +296,6 @@ impl fmt::Debug for Key { } impl Key { - /// The algorithm this key signs and verifies with. - pub fn algorithm(&self) -> Algorithm { - self.algorithm - } - - /// The operations this key is allowed to perform. - pub fn operations(&self) -> &HashSet { - &self.operations - } - - /// The key material, including its JWK type. - pub fn key_type(&self) -> &KeyType { - &self.key - } - - /// The key ID (`kid`), used to select this key out of a [`crate::KeySet`]. - pub fn kid(&self) -> Option<&crate::KeyId> { - self.kid.as_ref() - } - - /// The authorization ceiling on tokens this key signs or verifies, if any. - pub fn scope(&self) -> Option<&crate::Scope> { - self.scope.as_ref() - } - /// Parse a key from a string, auto-detecting JSON or base64url encoding. #[allow(clippy::should_implement_trait)] pub fn from_str(s: &str) -> crate::Result { @@ -396,11 +369,13 @@ impl Key { }; Ok(Self { - algorithm: self.algorithm, - operations: [KeyOperation::Verify].into(), - key, - kid: self.kid.clone(), - scope: self.scope.clone(), + jwk: Jwk { + algorithm: self.algorithm, + operations: [KeyOperation::Verify].into(), + key, + kid: self.kid.clone(), + scope: self.scope.clone(), + }, decode: Default::default(), encode: Default::default(), }) @@ -603,13 +578,13 @@ impl Key { /// scope that permits nothing. pub fn with_scope(mut self, scope: crate::Scope) -> crate::Result { scope.validate()?; - self.scope = Some(scope); + self.jwk.scope = Some(scope); Ok(self) } /// Derive a key restricted to the given operations. pub fn with_operations(mut self, operations: impl IntoIterator) -> Self { - self.operations = operations.into_iter().collect(); + self.jwk.operations = operations.into_iter().collect(); self } @@ -712,15 +687,15 @@ mod tests { let json = key.to_str().unwrap(); let loaded_key = Key::from_str(&json).unwrap(); - assert_eq!(loaded_key.algorithm(), key.algorithm()); - assert_eq!(loaded_key.operations(), key.operations()); - match (loaded_key.key_type(), key.key_type()) { + assert_eq!(loaded_key.algorithm, key.algorithm); + assert_eq!(loaded_key.operations, key.operations); + match (&loaded_key.key, &key.key) { (KeyType::OCT { secret: loaded_secret }, KeyType::OCT { secret }) => { assert_eq!(loaded_secret, secret); } _ => panic!("Expected OCT key"), } - assert_eq!(loaded_key.kid(), key.kid()); + assert_eq!(loaded_key.kid, key.kid); } /// Tests whether Key::from_str() works for keys without a kty value to fall back to OCT @@ -732,7 +707,7 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - if let KeyType::OCT { secret, .. } = key.key_type() { + if let KeyType::OCT { secret, .. } = &key.key { let base64_key = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(secret); assert_eq!(base64_key, "Fp8kipWUJeUFqeSqWym_tRC_tyI8z-QpqopIGrbrD68"); } else { @@ -743,10 +718,10 @@ mod tests { // Round-trip through from_str and verify fields let loaded = Key::from_str(&key_str).unwrap(); - assert_eq!(loaded.algorithm(), Algorithm::HS256); - assert!(loaded.operations().contains(&KeyOperation::Sign)); - assert!(loaded.operations().contains(&KeyOperation::Verify)); - assert!(matches!(loaded.key_type(), KeyType::OCT { .. })); + assert_eq!(loaded.algorithm, Algorithm::HS256); + assert!(loaded.operations.contains(&KeyOperation::Sign)); + assert!(loaded.operations.contains(&KeyOperation::Verify)); + assert!(matches!(loaded.key, KeyType::OCT { .. })); } #[test] @@ -765,10 +740,10 @@ mod tests { // Round-trip through from_str let loaded = Key::from_str(&encoded).unwrap(); - assert_eq!(loaded.algorithm(), Algorithm::HS256); - assert_eq!(loaded.kid(), key.kid()); - assert!(loaded.operations().contains(&KeyOperation::Sign)); - assert!(loaded.operations().contains(&KeyOperation::Verify)); + assert_eq!(loaded.algorithm, Algorithm::HS256); + assert_eq!(loaded.kid, key.kid); + assert!(loaded.operations.contains(&KeyOperation::Sign)); + assert!(loaded.operations.contains(&KeyOperation::Verify)); } #[test] @@ -848,7 +823,7 @@ mod tests { #[test] fn test_key_scope_requires_validation() { let key = create_test_key(); - assert!(key.scope().is_none()); + assert!(key.scope.is_none()); let useless = crate::Scope::default(); assert!(matches!( @@ -994,11 +969,11 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert_eq!(key.algorithm(), Algorithm::HS256); - assert_eq!(key.kid(), Some(&crate::KeyId::decode("test-id").unwrap())); - assert_eq!(key.operations(), &[KeyOperation::Sign, KeyOperation::Verify].into()); + assert_eq!(key.algorithm, Algorithm::HS256); + assert_eq!(key.kid, Some(crate::KeyId::decode("test-id").unwrap())); + assert_eq!(key.operations, [KeyOperation::Sign, KeyOperation::Verify].into()); - match key.key_type() { + match &key.key { KeyType::OCT { secret } => assert_eq!(secret.len(), 32), _ => panic!("Expected OCT key"), } @@ -1010,9 +985,9 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert_eq!(key.algorithm(), Algorithm::HS384); + assert_eq!(key.algorithm, Algorithm::HS384); - match key.key_type() { + match &key.key { KeyType::OCT { secret } => assert_eq!(secret.len(), 48), _ => panic!("Expected OCT key"), } @@ -1024,9 +999,9 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert_eq!(key.algorithm(), Algorithm::HS512); + assert_eq!(key.algorithm, Algorithm::HS512); - match key.key_type() { + match &key.key { KeyType::OCT { secret } => assert_eq!(secret.len(), 64), _ => panic!("Expected OCT key"), } @@ -1038,9 +1013,9 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert_eq!(key.algorithm(), Algorithm::RS512); - assert!(matches!(key.key_type(), KeyType::RSA { .. })); - match key.key_type() { + assert_eq!(key.algorithm, Algorithm::RS512); + assert!(matches!(key.key, KeyType::RSA { .. })); + match &key.key { KeyType::RSA { public, private } => { assert!(private.is_some()); assert_eq!(public.n.len(), 256); @@ -1056,8 +1031,8 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert_eq!(key.algorithm(), Algorithm::ES256); - assert!(matches!(key.key_type(), KeyType::EC { .. })) + assert_eq!(key.algorithm, Algorithm::ES256); + assert!(matches!(key.key, KeyType::EC { .. })) } #[test] @@ -1066,8 +1041,8 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert_eq!(key.algorithm(), Algorithm::PS512); - assert!(matches!(key.key_type(), KeyType::RSA { .. })); + assert_eq!(key.algorithm, Algorithm::PS512); + assert!(matches!(key.key, KeyType::RSA { .. })); } #[test] @@ -1076,8 +1051,8 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert_eq!(key.algorithm(), Algorithm::EdDSA); - assert!(matches!(key.key_type(), KeyType::OKP { .. })); + assert_eq!(key.algorithm, Algorithm::EdDSA); + assert!(matches!(key.key, KeyType::OKP { .. })); } #[test] @@ -1086,9 +1061,9 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert_eq!(key.algorithm(), Algorithm::HS256); - assert_eq!(key.kid(), None); - assert_eq!(key.operations(), &[KeyOperation::Sign, KeyOperation::Verify].into()); + assert_eq!(key.algorithm, Algorithm::HS256); + assert_eq!(key.kid, None); + assert_eq!(key.operations, [KeyOperation::Sign, KeyOperation::Verify].into()); } #[test] @@ -1106,16 +1081,16 @@ mod tests { let key = key.unwrap(); let public_key = key.to_public().unwrap(); - assert_eq!(key.kid(), public_key.kid()); - assert_eq!(public_key.operations(), &[KeyOperation::Verify].into()); + assert_eq!(key.kid, public_key.kid); + assert_eq!(public_key.operations, [KeyOperation::Verify].into()); assert!(public_key.encode.get().is_none()); assert!(public_key.decode.get().is_none()); - assert!(matches!(public_key.key_type(), KeyType::RSA { .. })); + assert!(matches!(public_key.key, KeyType::RSA { .. })); - if let KeyType::RSA { public, private } = public_key.key_type() { + if let KeyType::RSA { public, private } = &public_key.key { assert!(private.is_none()); - if let KeyType::RSA { public: src_public, .. } = key.key_type() { + if let KeyType::RSA { public: src_public, .. } = &key.key { assert_eq!(public.e, src_public.e); assert_eq!(public.n, src_public.n); } else { @@ -1133,13 +1108,13 @@ mod tests { let key = key.unwrap(); let public_key = key.to_public().unwrap(); - assert_eq!(key.kid(), public_key.kid()); - assert_eq!(public_key.operations(), &[KeyOperation::Verify].into()); + assert_eq!(key.kid, public_key.kid); + assert_eq!(public_key.operations, [KeyOperation::Verify].into()); assert!(public_key.encode.get().is_none()); assert!(public_key.decode.get().is_none()); - assert!(matches!(public_key.key_type(), KeyType::EC { .. })); + assert!(matches!(public_key.key, KeyType::EC { .. })); - if let KeyType::EC { x, y, d, curve } = public_key.key_type() { + if let KeyType::EC { x, y, d, curve } = &public_key.key { assert!(d.is_none()); if let KeyType::EC { @@ -1147,7 +1122,7 @@ mod tests { y: src_y, curve: src_curve, .. - } = key.key_type() + } = &key.key { assert_eq!(x, src_x); assert_eq!(y, src_y); @@ -1167,20 +1142,20 @@ mod tests { let key = key.unwrap(); let public_key = key.to_public().unwrap(); - assert_eq!(key.kid(), public_key.kid()); - assert_eq!(public_key.operations(), &[KeyOperation::Verify].into()); + assert_eq!(key.kid, public_key.kid); + assert_eq!(public_key.operations, [KeyOperation::Verify].into()); assert!(public_key.encode.get().is_none()); assert!(public_key.decode.get().is_none()); - assert!(matches!(public_key.key_type(), KeyType::OKP { .. })); + assert!(matches!(public_key.key, KeyType::OKP { .. })); - if let KeyType::OKP { x, d, curve } = public_key.key_type() { + if let KeyType::OKP { x, d, curve } = &public_key.key { assert!(d.is_none()); if let KeyType::OKP { x: src_x, curve: src_curve, .. - } = key.key_type() + } = &key.key { assert_eq!(x, src_x); assert_eq!(curve, src_curve); @@ -1252,9 +1227,9 @@ mod tests { let json = serde_json::to_string(&key).unwrap(); let deserialized: Key = serde_json::from_str(&json).unwrap(); - assert_eq!(deserialized.algorithm(), key.algorithm()); - assert_eq!(deserialized.operations(), key.operations()); - assert_eq!(deserialized.kid(), key.kid()); + assert_eq!(deserialized.algorithm, key.algorithm); + assert_eq!(deserialized.operations, key.operations); + assert_eq!(deserialized.kid, key.kid); if let ( KeyType::OCT { @@ -1263,7 +1238,7 @@ mod tests { KeyType::OCT { secret: deserialized_secret, }, - ) = (key.key_type(), deserialized.key_type()) + ) = (&key.key, &deserialized.key) { assert_eq!(deserialized_secret, original_secret); } else { @@ -1276,16 +1251,16 @@ mod tests { let key = create_test_key(); let cloned = key.clone(); - assert_eq!(cloned.algorithm(), key.algorithm()); - assert_eq!(cloned.operations(), key.operations()); - assert_eq!(cloned.kid(), key.kid()); + assert_eq!(cloned.algorithm, key.algorithm); + assert_eq!(cloned.operations, key.operations); + assert_eq!(cloned.kid, key.kid); if let ( KeyType::OCT { secret: original_secret, }, KeyType::OCT { secret: cloned_secret }, - ) = (key.key_type(), cloned.key_type()) + ) = (&key.key, &cloned.key) { assert_eq!(cloned_secret, original_secret); } else { @@ -1412,20 +1387,20 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert!(key.operations().contains(&KeyOperation::Sign)); - assert!(key.operations().contains(&KeyOperation::Verify)); + assert!(key.operations.contains(&KeyOperation::Sign)); + assert!(key.operations.contains(&KeyOperation::Verify)); let public_key = key.to_public().unwrap(); - assert!(!public_key.operations().contains(&KeyOperation::Sign)); - assert!(public_key.operations().contains(&KeyOperation::Verify)); + assert!(!public_key.operations.contains(&KeyOperation::Sign)); + assert!(public_key.operations.contains(&KeyOperation::Verify)); - match key.key_type() { + match &key.key { KeyType::RSA { public, private } => { assert!(private.is_some()); assert_eq!(public.n.len(), 256); assert_eq!(public.e.len(), 3); - match public_key.key_type() { + match &public_key.key { KeyType::RSA { public: guest_public, private: public_private, @@ -1447,20 +1422,20 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - assert!(key.operations().contains(&KeyOperation::Sign)); - assert!(key.operations().contains(&KeyOperation::Verify)); + assert!(key.operations.contains(&KeyOperation::Sign)); + assert!(key.operations.contains(&KeyOperation::Verify)); let public_key = key.to_public().unwrap(); - assert!(!public_key.operations().contains(&KeyOperation::Sign)); - assert!(public_key.operations().contains(&KeyOperation::Verify)); + assert!(!public_key.operations.contains(&KeyOperation::Sign)); + assert!(public_key.operations.contains(&KeyOperation::Verify)); - match key.key_type() { + match &key.key { KeyType::RSA { public, private } => { assert!(private.is_some()); assert_eq!(public.n.len(), 256); assert_eq!(public.e.len(), 3); - match public_key.key_type() { + match &public_key.key { KeyType::RSA { public: guest_public, private: public_private, @@ -1497,7 +1472,7 @@ mod tests { if let KeyType::OCT { secret: original_secret, - } = key.key_type() + } = &key.key { assert_eq!(decoded, *original_secret); } else { @@ -1512,10 +1487,10 @@ mod tests { // Should be able to deserialize new format let key: Key = serde_json::from_str(unpadded_json).unwrap(); - assert_eq!(key.algorithm(), Algorithm::HS256); - assert_eq!(key.kid(), Some(&crate::KeyId::decode("test-key-1").unwrap())); + assert_eq!(key.algorithm, Algorithm::HS256); + assert_eq!(key.kid, Some(crate::KeyId::decode("test-key-1").unwrap())); - if let KeyType::OCT { secret } = key.key_type() { + if let KeyType::OCT { secret } = &key.key { assert_eq!(secret, b"test-secret-that-is-long-enough-for-hmac-sha256"); } else { panic!("Expected key to be OCT variant"); @@ -1529,10 +1504,10 @@ mod tests { // Should be able to deserialize old format for backwards compatibility let key: Key = serde_json::from_str(padded_json).unwrap(); - assert_eq!(key.algorithm(), Algorithm::HS256); - assert_eq!(key.kid(), Some(&crate::KeyId::decode("test-key-1").unwrap())); + assert_eq!(key.algorithm, Algorithm::HS256); + assert_eq!(key.kid, Some(crate::KeyId::decode("test-key-1").unwrap())); - if let KeyType::OCT { secret } = key.key_type() { + if let KeyType::OCT { secret } = &key.key { assert_eq!(secret, b"test-secret-that-is-long-enough-for-hmac-sha256"); } else { panic!("Expected key to be OCT variant"); @@ -1563,8 +1538,8 @@ mod tests { #[test] fn test_js_hs256_key_load() { let key = Key::from_str(JS_HS256_KEY).unwrap(); - assert_eq!(key.algorithm(), Algorithm::HS256); - assert_eq!(key.kid(), Some(&crate::KeyId::decode("js-test-key").unwrap())); + assert_eq!(key.algorithm, Algorithm::HS256); + assert_eq!(key.kid, Some(crate::KeyId::decode("js-test-key").unwrap())); } #[test] @@ -1594,11 +1569,11 @@ mod tests { #[test] fn test_js_eddsa_key_load() { let private_key = Key::from_str(JS_EDDSA_PRIVATE_KEY).unwrap(); - assert_eq!(private_key.algorithm(), Algorithm::EdDSA); - assert!(matches!(private_key.key_type(), KeyType::OKP { .. })); + assert_eq!(private_key.algorithm, Algorithm::EdDSA); + assert!(matches!(private_key.key, KeyType::OKP { .. })); let public_key = Key::from_str(JS_EDDSA_PUBLIC_KEY).unwrap(); - assert_eq!(public_key.algorithm(), Algorithm::EdDSA); + assert_eq!(public_key.algorithm, Algorithm::EdDSA); } #[test] @@ -1637,7 +1612,7 @@ mod tests { fn test_file_io_base64url() { let key = create_test_key(); let temp_dir = std::env::temp_dir(); - let temp_path = temp_dir.join("test_jwk.key_type()"); + let temp_path = temp_dir.join("test_jwk.key"); // Write key to file as base64url key.to_file(&temp_path).unwrap(); @@ -1659,16 +1634,16 @@ mod tests { // Read key back from file let loaded_key = Key::from_file(&temp_path).unwrap(); - assert_eq!(loaded_key.algorithm(), key.algorithm()); - assert_eq!(loaded_key.operations(), key.operations()); - assert_eq!(loaded_key.kid(), key.kid()); + assert_eq!(loaded_key.algorithm, key.algorithm); + assert_eq!(loaded_key.operations, key.operations); + assert_eq!(loaded_key.kid, key.kid); if let ( KeyType::OCT { secret: original_secret, }, KeyType::OCT { secret: loaded_secret }, - ) = (key.key_type(), loaded_key.key_type()) + ) = (&key.key, &loaded_key.key) { assert_eq!(loaded_secret, original_secret); } else { @@ -1683,7 +1658,7 @@ mod tests { fn test_file_io_raw_json() { let key = create_test_key(); let temp_dir = std::env::temp_dir(); - let temp_path = temp_dir.join("test_jwk_raw_json.key_type()"); + let temp_path = temp_dir.join("test_jwk_raw_json.key"); // Write key as raw JSON (backwards compat format) let json = serde_json::to_string(&key).unwrap(); @@ -1694,16 +1669,16 @@ mod tests { // Load via from_file (should auto-detect JSON) let loaded_key = Key::from_file(&temp_path).unwrap(); - assert_eq!(loaded_key.algorithm(), key.algorithm()); - assert_eq!(loaded_key.operations(), key.operations()); - assert_eq!(loaded_key.kid(), key.kid()); + assert_eq!(loaded_key.algorithm, key.algorithm); + assert_eq!(loaded_key.operations, key.operations); + assert_eq!(loaded_key.kid, key.kid); if let ( KeyType::OCT { secret: original_secret, }, KeyType::OCT { secret: loaded_secret }, - ) = (key.key_type(), loaded_key.key_type()) + ) = (&key.key, &loaded_key.key) { assert_eq!(loaded_secret, original_secret); } else { diff --git a/rs/moq-token/src/set.rs b/rs/moq-token/src/set.rs index 933d085cd8..5416dad895 100644 --- a/rs/moq-token/src/set.rs +++ b/rs/moq-token/src/set.rs @@ -81,16 +81,13 @@ impl KeySet { pub fn find_key(&self, kid: &str) -> Option> { self.keys .iter() - .find(|k| k.kid().is_some_and(|k| k.encode() == kid)) + .find(|k| k.kid.as_ref().is_some_and(|k| k.encode() == kid)) .cloned() } /// Find the first key that supports the given operation. pub fn find_supported_key(&self, operation: &KeyOperation) -> Option> { - self.keys - .iter() - .find(|key| key.operations().contains(operation)) - .cloned() + self.keys.iter().find(|key| key.operations.contains(operation)).cloned() } /// Sign the claims with the first key in the set that supports signing. @@ -174,7 +171,7 @@ mod tests { assert!(set.is_ok()); let set = set.unwrap(); assert_eq!(set.keys.len(), 1); - assert_eq!(set.keys[0].kid().map(|k| k.encode()), Some("1")); + assert_eq!(set.keys[0].kid.as_ref().map(|k| k.encode()), Some("1")); assert!(set.find_key("1").is_some()); } @@ -228,7 +225,7 @@ mod tests { let found = set.find_key("my-key"); assert!(found.is_some()); - assert_eq!(found.unwrap().kid().map(|k| k.encode()), Some("my-key")); + assert_eq!(found.unwrap().kid.as_ref().map(|k| k.encode()), Some("my-key")); } #[test] @@ -264,11 +261,11 @@ mod tests { let found_sign = set.find_supported_key(&KeyOperation::Sign); assert!(found_sign.is_some()); - assert_eq!(found_sign.unwrap().kid().map(|k| k.encode()), Some("sign")); + assert_eq!(found_sign.unwrap().kid.as_ref().map(|k| k.encode()), Some("sign")); let found_verify = set.find_supported_key(&KeyOperation::Verify); assert!(found_verify.is_some()); - assert_eq!(found_verify.unwrap().kid().map(|k| k.encode()), Some("verify")); + assert_eq!(found_verify.unwrap().kid.as_ref().map(|k| k.encode()), Some("verify")); } #[test] @@ -284,9 +281,9 @@ mod tests { assert_eq!(public_set.keys.len(), 1); let public_key = &public_set.keys[0]; - assert_eq!(public_key.kid().map(|k| k.encode()), Some("1")); - assert!(public_key.operations().contains(&KeyOperation::Verify)); - assert!(!public_key.operations().contains(&KeyOperation::Sign)); + assert_eq!(public_key.kid.as_ref().map(|k| k.encode()), Some("1")); + assert!(public_key.operations.contains(&KeyOperation::Verify)); + assert!(!public_key.operations.contains(&KeyOperation::Sign)); } #[test] @@ -417,7 +414,7 @@ mod tests { let loaded = KeySet::from_file(&path).expect("failed to read from file"); assert_eq!(loaded.keys.len(), 1); - assert_eq!(loaded.keys[0].kid().map(|k| k.encode()), Some("1")); + assert_eq!(loaded.keys[0].kid.as_ref().map(|k| k.encode()), Some("1")); // Clean up let _ = std::fs::remove_file(path); From c6f3feb532194e3dc0b3fd8528824c4f1eb54247 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 22 Jul 2026 08:53:59 -0700 Subject: [PATCH 08/11] refactor(token)!: name the key material and give Jwk a named conversion `Key` derefs to its `Jwk`, so the JWK field names are now user-facing, and `key.key` stuttered. The type it holds is not a "type" either: it carries the actual EC/RSA/OCT/OKP parameters, private ones included. Renames `KeyType` to `KeyMaterial` and the field to `material`, so it reads `key.material`. Adds `Jwk::validate(self) -> Result` as the named counterpart to `Key::try_from`, so the conversion is discoverable from the `Jwk` docs instead of only through a trait impl. `TryFrom` delegates to it. Keeps the `Jwk` and `Key` names. `Jwk` is precise and matches RFC 7517; the friendlier alternatives are either vaguer or, in the case of `KeyInfo`, actively misleading, since unlike the `Info` types in moq-net this one carries private key material and must not read as harmless metadata. No wire change: `material` is `#[serde(flatten)]` and the other fields are serde-renamed, so the JWK JSON is untouched. The JS interop tests, which parse @moq/token-generated keys and verify JS-signed tokens, still pass. Co-Authored-By: Claude Opus 4.8 --- rs/moq-token/src/generate.rs | 18 ++-- rs/moq-token/src/key.rs | 189 ++++++++++++++++++----------------- 2 files changed, 106 insertions(+), 101 deletions(-) diff --git a/rs/moq-token/src/generate.rs b/rs/moq-token/src/generate.rs index 27e3cdcc83..894717a6f7 100644 --- a/rs/moq-token/src/generate.rs +++ b/rs/moq-token/src/generate.rs @@ -1,5 +1,5 @@ use crate::error::KeyError; -use crate::{Algorithm, EllipticCurve, Jwk, Key, KeyType, RsaPublicKey}; +use crate::{Algorithm, EllipticCurve, Jwk, Key, KeyMaterial, RsaPublicKey}; use aws_lc_rs::encoding::AsBigEndian; use aws_lc_rs::signature::KeyPair; use p256::elliptic_curve::array::typenum::Unsigned; @@ -26,10 +26,10 @@ pub fn generate(algorithm: Algorithm, id: Option) -> crate::Result jwk.try_into() } -fn generate_hmac_key() -> crate::Result { +fn generate_hmac_key() -> crate::Result { let mut key = [0u8; SIZE]; aws_lc_rs::rand::fill(&mut key)?; - Ok(KeyType::OCT { secret: key.to_vec() }) + Ok(KeyMaterial::OCT { secret: key.to_vec() }) } struct AwsRng; @@ -58,12 +58,12 @@ impl rsa::rand_core::RngCore for AwsRng { impl rsa::rand_core::CryptoRng for AwsRng {} -fn generate_rsa_key(size: usize) -> crate::Result { +fn generate_rsa_key(size: usize) -> crate::Result { let mut rng = AwsRng; let mut key = rsa::RsaPrivateKey::new(&mut rng, size)?; key.precompute()?; - Ok(KeyType::RSA { + Ok(KeyMaterial::RSA { public: RsaPublicKey { e: key.e().to_bytes_be(), n: key.n().to_bytes_be(), @@ -80,7 +80,7 @@ fn generate_rsa_key(size: usize) -> crate::Result { }) } -fn generate_ec_key(curve: EllipticCurve) -> crate::Result +fn generate_ec_key(curve: EllipticCurve) -> crate::Result where C: Curve + CurveArithmetic + PointCompression, C::AffinePoint: ToSec1Point + FromSec1Point, @@ -101,7 +101,7 @@ where let y = point.y().ok_or(KeyError::MissingEcY)?.to_vec(); let d = secret.to_bytes().to_vec(); - Ok(KeyType::EC { + Ok(KeyMaterial::EC { curve, x, y, @@ -109,13 +109,13 @@ where }) } -fn generate_ed25519_key() -> crate::Result { +fn generate_ed25519_key() -> crate::Result { let key_pair = aws_lc_rs::signature::Ed25519KeyPair::generate()?; let public_key = key_pair.public_key().as_ref().to_vec(); let seed = key_pair.seed()?.as_be_bytes()?.as_ref().to_vec(); - Ok(KeyType::OKP { + Ok(KeyMaterial::OKP { curve: EllipticCurve::Ed25519, x: public_key, d: Some(seed), diff --git a/rs/moq-token/src/key.rs b/rs/moq-token/src/key.rs index 8dc21da5a8..42e5f6c113 100644 --- a/rs/moq-token/src/key.rs +++ b/rs/moq-token/src/key.rs @@ -24,7 +24,7 @@ pub enum KeyOperation { /// #[derive(Clone, Serialize, Deserialize)] #[serde(tag = "kty")] -pub enum KeyType { +pub enum KeyMaterial { /// EC { #[serde(rename = "crv")] @@ -156,9 +156,9 @@ pub struct Jwk { #[serde(rename = "key_ops")] pub operations: HashSet, - /// The key material. Defaults to [`KeyType::OCT`] when `kty` is absent. + /// The key material. Defaults to [`KeyMaterial::OCT`] when `kty` is absent. #[serde(flatten)] - pub key: KeyType, + pub material: KeyMaterial, /// The key ID, useful for rotating keys. #[serde(skip_serializing_if = "Option::is_none")] @@ -174,15 +174,31 @@ impl Jwk { /// /// Set the remaining fields on the returned value. The struct is `#[non_exhaustive]`, so /// building it this way keeps working as JWK parameters are added. - pub fn new(algorithm: Algorithm, key: KeyType) -> Self { + pub fn new(algorithm: Algorithm, material: KeyMaterial) -> Self { Self { algorithm, operations: [KeyOperation::Sign, KeyOperation::Verify].into(), - key, + material, kid: None, scope: None, } } + + /// Check the parameters and turn this into a [`Key`] that can sign and verify. + /// + /// The named counterpart to `Key::try_from`, so the conversion is discoverable from here + /// rather than only from a trait impl. + pub fn validate(self) -> crate::Result { + if let Some(scope) = &self.scope { + scope.validate()?; + } + + Ok(Key { + jwk: self, + decode: Default::default(), + encode: Default::default(), + }) + } } impl<'de> Deserialize<'de> for Jwk { @@ -245,15 +261,7 @@ impl TryFrom for Key { type Error = crate::Error; fn try_from(jwk: Jwk) -> crate::Result { - if let Some(scope) = &jwk.scope { - scope.validate()?; - } - - Ok(Self { - jwk, - decode: Default::default(), - encode: Default::default(), - }) + jwk.validate() } } @@ -344,24 +352,24 @@ impl Key { return Err(KeyError::VerifyUnsupported.into()); } - let key = match self.key { - KeyType::RSA { ref public, .. } => KeyType::RSA { + let material = match self.material { + KeyMaterial::RSA { ref public, .. } => KeyMaterial::RSA { public: public.clone(), private: None, }, - KeyType::EC { + KeyMaterial::EC { ref x, ref y, ref curve, .. - } => KeyType::EC { + } => KeyMaterial::EC { x: x.clone(), y: y.clone(), curve: curve.clone(), d: None, }, - KeyType::OCT { .. } => return Err(KeyError::NoPublicKey.into()), - KeyType::OKP { ref x, ref curve, .. } => KeyType::OKP { + KeyMaterial::OCT { .. } => return Err(KeyError::NoPublicKey.into()), + KeyMaterial::OKP { ref x, ref curve, .. } => KeyMaterial::OKP { x: x.clone(), curve: curve.clone(), d: None, @@ -372,7 +380,7 @@ impl Key { jwk: Jwk { algorithm: self.algorithm, operations: [KeyOperation::Verify].into(), - key, + material, kid: self.kid.clone(), scope: self.scope.clone(), }, @@ -386,12 +394,12 @@ impl Key { return Ok(key); } - let decoding_key = match self.key { - KeyType::OCT { ref secret } => match self.algorithm { + let decoding_key = match self.material { + KeyMaterial::OCT { ref secret } => match self.algorithm { Algorithm::HS256 | Algorithm::HS384 | Algorithm::HS512 => DecodingKey::from_secret(secret), _ => return Err(KeyError::InvalidAlgorithm.into()), }, - KeyType::EC { + KeyMaterial::EC { ref curve, ref x, ref y, @@ -425,7 +433,7 @@ impl Key { } _ => return Err(KeyError::InvalidCurve("EC").into()), }, - KeyType::OKP { ref curve, ref x, .. } => match curve { + KeyMaterial::OKP { ref curve, ref x, .. } => match curve { EllipticCurve::Ed25519 => { if self.algorithm != Algorithm::EdDSA { return Err(KeyError::InvalidAlgorithmForCurve("Ed25519").into()); @@ -437,7 +445,7 @@ impl Key { } _ => return Err(KeyError::InvalidCurve("OKP").into()), }, - KeyType::RSA { ref public, .. } => { + KeyMaterial::RSA { ref public, .. } => { DecodingKey::from_rsa_raw_components(public.n.as_ref(), public.e.as_ref()) } }; @@ -450,12 +458,12 @@ impl Key { return Ok(key); } - let encoding_key = match self.key { - KeyType::OCT { ref secret } => match self.algorithm { + let encoding_key = match self.material { + KeyMaterial::OCT { ref secret } => match self.algorithm { Algorithm::HS256 | Algorithm::HS384 | Algorithm::HS512 => EncodingKey::from_secret(secret), _ => return Err(KeyError::InvalidAlgorithm.into()), }, - KeyType::EC { ref curve, ref d, .. } => { + KeyMaterial::EC { ref curve, ref d, .. } => { let d = d.as_ref().ok_or(KeyError::MissingPrivateKey)?; match curve { @@ -472,7 +480,7 @@ impl Key { _ => return Err(KeyError::InvalidCurve("EC").into()), } } - KeyType::OKP { + KeyMaterial::OKP { ref curve, ref d, ref x, @@ -487,7 +495,7 @@ impl Key { _ => return Err(KeyError::InvalidCurve("OKP").into()), } } - KeyType::RSA { + KeyMaterial::RSA { ref public, ref private, } => { @@ -658,17 +666,14 @@ mod tests { use std::time::{Duration, SystemTime}; fn create_test_key() -> Key { - Jwk { - algorithm: Algorithm::HS256, - operations: [KeyOperation::Sign, KeyOperation::Verify].into(), - key: KeyType::OCT { + let mut jwk = Jwk::new( + Algorithm::HS256, + KeyMaterial::OCT { secret: b"test-secret-that-is-long-enough-for-hmac-sha256".to_vec(), }, - kid: Some(crate::KeyId::decode("test-key-1").unwrap()), - scope: None, - } - .try_into() - .unwrap() + ); + jwk.kid = Some(crate::KeyId::decode("test-key-1").unwrap()); + jwk.validate().unwrap() } fn create_test_claims() -> Claims { @@ -689,8 +694,8 @@ mod tests { assert_eq!(loaded_key.algorithm, key.algorithm); assert_eq!(loaded_key.operations, key.operations); - match (&loaded_key.key, &key.key) { - (KeyType::OCT { secret: loaded_secret }, KeyType::OCT { secret }) => { + match (&loaded_key.material, &key.material) { + (KeyMaterial::OCT { secret: loaded_secret }, KeyMaterial::OCT { secret }) => { assert_eq!(loaded_secret, secret); } _ => panic!("Expected OCT key"), @@ -707,7 +712,7 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - if let KeyType::OCT { secret, .. } = &key.key { + if let KeyMaterial::OCT { secret, .. } = &key.material { let base64_key = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(secret); assert_eq!(base64_key, "Fp8kipWUJeUFqeSqWym_tRC_tyI8z-QpqopIGrbrD68"); } else { @@ -721,7 +726,7 @@ mod tests { assert_eq!(loaded.algorithm, Algorithm::HS256); assert!(loaded.operations.contains(&KeyOperation::Sign)); assert!(loaded.operations.contains(&KeyOperation::Verify)); - assert!(matches!(loaded.key, KeyType::OCT { .. })); + assert!(matches!(loaded.material, KeyMaterial::OCT { .. })); } #[test] @@ -973,8 +978,8 @@ mod tests { assert_eq!(key.kid, Some(crate::KeyId::decode("test-id").unwrap())); assert_eq!(key.operations, [KeyOperation::Sign, KeyOperation::Verify].into()); - match &key.key { - KeyType::OCT { secret } => assert_eq!(secret.len(), 32), + match &key.material { + KeyMaterial::OCT { secret } => assert_eq!(secret.len(), 32), _ => panic!("Expected OCT key"), } } @@ -987,8 +992,8 @@ mod tests { assert_eq!(key.algorithm, Algorithm::HS384); - match &key.key { - KeyType::OCT { secret } => assert_eq!(secret.len(), 48), + match &key.material { + KeyMaterial::OCT { secret } => assert_eq!(secret.len(), 48), _ => panic!("Expected OCT key"), } } @@ -1001,8 +1006,8 @@ mod tests { assert_eq!(key.algorithm, Algorithm::HS512); - match &key.key { - KeyType::OCT { secret } => assert_eq!(secret.len(), 64), + match &key.material { + KeyMaterial::OCT { secret } => assert_eq!(secret.len(), 64), _ => panic!("Expected OCT key"), } } @@ -1014,9 +1019,9 @@ mod tests { let key = key.unwrap(); assert_eq!(key.algorithm, Algorithm::RS512); - assert!(matches!(key.key, KeyType::RSA { .. })); - match &key.key { - KeyType::RSA { public, private } => { + assert!(matches!(key.material, KeyMaterial::RSA { .. })); + match &key.material { + KeyMaterial::RSA { public, private } => { assert!(private.is_some()); assert_eq!(public.n.len(), 256); assert_eq!(public.e.len(), 3); @@ -1032,7 +1037,7 @@ mod tests { let key = key.unwrap(); assert_eq!(key.algorithm, Algorithm::ES256); - assert!(matches!(key.key, KeyType::EC { .. })) + assert!(matches!(key.material, KeyMaterial::EC { .. })) } #[test] @@ -1042,7 +1047,7 @@ mod tests { let key = key.unwrap(); assert_eq!(key.algorithm, Algorithm::PS512); - assert!(matches!(key.key, KeyType::RSA { .. })); + assert!(matches!(key.material, KeyMaterial::RSA { .. })); } #[test] @@ -1052,7 +1057,7 @@ mod tests { let key = key.unwrap(); assert_eq!(key.algorithm, Algorithm::EdDSA); - assert!(matches!(key.key, KeyType::OKP { .. })); + assert!(matches!(key.material, KeyMaterial::OKP { .. })); } #[test] @@ -1085,12 +1090,12 @@ mod tests { assert_eq!(public_key.operations, [KeyOperation::Verify].into()); assert!(public_key.encode.get().is_none()); assert!(public_key.decode.get().is_none()); - assert!(matches!(public_key.key, KeyType::RSA { .. })); + assert!(matches!(public_key.material, KeyMaterial::RSA { .. })); - if let KeyType::RSA { public, private } = &public_key.key { + if let KeyMaterial::RSA { public, private } = &public_key.material { assert!(private.is_none()); - if let KeyType::RSA { public: src_public, .. } = &key.key { + if let KeyMaterial::RSA { public: src_public, .. } = &key.material { assert_eq!(public.e, src_public.e); assert_eq!(public.n, src_public.n); } else { @@ -1112,17 +1117,17 @@ mod tests { assert_eq!(public_key.operations, [KeyOperation::Verify].into()); assert!(public_key.encode.get().is_none()); assert!(public_key.decode.get().is_none()); - assert!(matches!(public_key.key, KeyType::EC { .. })); + assert!(matches!(public_key.material, KeyMaterial::EC { .. })); - if let KeyType::EC { x, y, d, curve } = &public_key.key { + if let KeyMaterial::EC { x, y, d, curve } = &public_key.material { assert!(d.is_none()); - if let KeyType::EC { + if let KeyMaterial::EC { x: src_x, y: src_y, curve: src_curve, .. - } = &key.key + } = &key.material { assert_eq!(x, src_x); assert_eq!(y, src_y); @@ -1146,16 +1151,16 @@ mod tests { assert_eq!(public_key.operations, [KeyOperation::Verify].into()); assert!(public_key.encode.get().is_none()); assert!(public_key.decode.get().is_none()); - assert!(matches!(public_key.key, KeyType::OKP { .. })); + assert!(matches!(public_key.material, KeyMaterial::OKP { .. })); - if let KeyType::OKP { x, d, curve } = &public_key.key { + if let KeyMaterial::OKP { x, d, curve } = &public_key.material { assert!(d.is_none()); - if let KeyType::OKP { + if let KeyMaterial::OKP { x: src_x, curve: src_curve, .. - } = &key.key + } = &key.material { assert_eq!(x, src_x); assert_eq!(curve, src_curve); @@ -1232,13 +1237,13 @@ mod tests { assert_eq!(deserialized.kid, key.kid); if let ( - KeyType::OCT { + KeyMaterial::OCT { secret: original_secret, }, - KeyType::OCT { + KeyMaterial::OCT { secret: deserialized_secret, }, - ) = (&key.key, &deserialized.key) + ) = (&key.material, &deserialized.material) { assert_eq!(deserialized_secret, original_secret); } else { @@ -1256,11 +1261,11 @@ mod tests { assert_eq!(cloned.kid, key.kid); if let ( - KeyType::OCT { + KeyMaterial::OCT { secret: original_secret, }, - KeyType::OCT { secret: cloned_secret }, - ) = (&key.key, &cloned.key) + KeyMaterial::OCT { secret: cloned_secret }, + ) = (&key.material, &cloned.material) { assert_eq!(cloned_secret, original_secret); } else { @@ -1394,14 +1399,14 @@ mod tests { assert!(!public_key.operations.contains(&KeyOperation::Sign)); assert!(public_key.operations.contains(&KeyOperation::Verify)); - match &key.key { - KeyType::RSA { public, private } => { + match &key.material { + KeyMaterial::RSA { public, private } => { assert!(private.is_some()); assert_eq!(public.n.len(), 256); assert_eq!(public.e.len(), 3); - match &public_key.key { - KeyType::RSA { + match &public_key.material { + KeyMaterial::RSA { public: guest_public, private: public_private, } => { @@ -1429,14 +1434,14 @@ mod tests { assert!(!public_key.operations.contains(&KeyOperation::Sign)); assert!(public_key.operations.contains(&KeyOperation::Verify)); - match &key.key { - KeyType::RSA { public, private } => { + match &key.material { + KeyMaterial::RSA { public, private } => { assert!(private.is_some()); assert_eq!(public.n.len(), 256); assert_eq!(public.e.len(), 3); - match &public_key.key { - KeyType::RSA { + match &public_key.material { + KeyMaterial::RSA { public: guest_public, private: public_private, } => { @@ -1470,9 +1475,9 @@ mod tests { .decode(k_value) .unwrap(); - if let KeyType::OCT { + if let KeyMaterial::OCT { secret: original_secret, - } = &key.key + } = &key.material { assert_eq!(decoded, *original_secret); } else { @@ -1490,7 +1495,7 @@ mod tests { assert_eq!(key.algorithm, Algorithm::HS256); assert_eq!(key.kid, Some(crate::KeyId::decode("test-key-1").unwrap())); - if let KeyType::OCT { secret } = &key.key { + if let KeyMaterial::OCT { secret } = &key.material { assert_eq!(secret, b"test-secret-that-is-long-enough-for-hmac-sha256"); } else { panic!("Expected key to be OCT variant"); @@ -1507,7 +1512,7 @@ mod tests { assert_eq!(key.algorithm, Algorithm::HS256); assert_eq!(key.kid, Some(crate::KeyId::decode("test-key-1").unwrap())); - if let KeyType::OCT { secret } = &key.key { + if let KeyMaterial::OCT { secret } = &key.material { assert_eq!(secret, b"test-secret-that-is-long-enough-for-hmac-sha256"); } else { panic!("Expected key to be OCT variant"); @@ -1570,7 +1575,7 @@ mod tests { fn test_js_eddsa_key_load() { let private_key = Key::from_str(JS_EDDSA_PRIVATE_KEY).unwrap(); assert_eq!(private_key.algorithm, Algorithm::EdDSA); - assert!(matches!(private_key.key, KeyType::OKP { .. })); + assert!(matches!(private_key.material, KeyMaterial::OKP { .. })); let public_key = Key::from_str(JS_EDDSA_PUBLIC_KEY).unwrap(); assert_eq!(public_key.algorithm, Algorithm::EdDSA); @@ -1639,11 +1644,11 @@ mod tests { assert_eq!(loaded_key.kid, key.kid); if let ( - KeyType::OCT { + KeyMaterial::OCT { secret: original_secret, }, - KeyType::OCT { secret: loaded_secret }, - ) = (&key.key, &loaded_key.key) + KeyMaterial::OCT { secret: loaded_secret }, + ) = (&key.material, &loaded_key.material) { assert_eq!(loaded_secret, original_secret); } else { @@ -1674,11 +1679,11 @@ mod tests { assert_eq!(loaded_key.kid, key.kid); if let ( - KeyType::OCT { + KeyMaterial::OCT { secret: original_secret, }, - KeyType::OCT { secret: loaded_secret }, - ) = (&key.key, &loaded_key.key) + KeyMaterial::OCT { secret: loaded_secret }, + ) = (&key.material, &loaded_key.material) { assert_eq!(loaded_secret, original_secret); } else { From e9ab88e152ccf56b54f5f3180bd993cc93e0f03d Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 22 Jul 2026 09:07:06 -0700 Subject: [PATCH 09/11] fix(watch): report the real container kind when skipping a rendition Tagging the passthrough arm `{ kind: "unknown", raw }` made the skip warning print the literal "unknown" instead of the kind the publisher actually named, which was the one fact the warning existed to convey. Read it back out of `raw`. Also corrects three docs the rename and the tagging left behind: the `UnknownContainerSchema` rationale still claimed the parsed value round-trips directly (it round-trips through `raw`), `rs/CLAUDE.md` still named `KeyType`, and the `Jwk` docs pointed only at `Key::try_from` rather than the named `Jwk::validate` added alongside it. Found by a review pass over the commits CodeRabbit had not seen. Co-Authored-By: Claude Opus 4.8 --- js/hang/src/catalog/container.ts | 4 ++-- js/watch/src/audio/decoder.ts | 4 +++- js/watch/src/video/decoder.ts | 4 +++- rs/CLAUDE.md | 2 +- rs/moq-token/src/key.rs | 3 ++- 5 files changed, 11 insertions(+), 6 deletions(-) diff --git a/js/hang/src/catalog/container.ts b/js/hang/src/catalog/container.ts index 2ab0493bea..be55f54ddf 100644 --- a/js/hang/src/catalog/container.ts +++ b/js/hang/src/catalog/container.ts @@ -6,8 +6,8 @@ const KNOWN_KINDS = ["legacy", "cmaf", "loc"]; /** * A container this build does not recognize, preserved verbatim. * - * Kept intact so reparsing and republishing a catalog round-trips the rendition instead of - * corrupting it. Such a rendition must be ignored rather than decoded. + * The original object is kept verbatim under `raw`, so nothing about the rendition is lost and a + * republisher can write it back out unchanged. Such a rendition must be ignored, not decoded. * * Recognized kinds are rejected here so they can only ever parse through their own strict * schema. Without that, a malformed known container (`{"kind":"cmaf"}` with no `init`) would diff --git a/js/watch/src/audio/decoder.ts b/js/watch/src/audio/decoder.ts index d8560261cd..2f23468508 100644 --- a/js/watch/src/audio/decoder.ts +++ b/js/watch/src/audio/decoder.ts @@ -514,7 +514,9 @@ export class Decoder { async function supported(config: Catalog.AudioConfig): Promise { if (!Catalog.containerSupported(config.container)) { - console.warn(`audio: ignoring rendition with unknown container: ${config.container.kind}`); + // `kind` is the literal "unknown" tag; the container the publisher actually named is in `raw`. + const kind = config.container.kind === "unknown" ? config.container.raw.kind : config.container.kind; + console.warn(`audio: ignoring rendition with unknown container: ${kind}`); return false; } diff --git a/js/watch/src/video/decoder.ts b/js/watch/src/video/decoder.ts index 09bf6cc375..b820b409f8 100644 --- a/js/watch/src/video/decoder.ts +++ b/js/watch/src/video/decoder.ts @@ -557,7 +557,9 @@ class DecoderTrack { async function supported(config: Catalog.VideoConfig): Promise { if (!Catalog.containerSupported(config.container)) { - console.warn(`video: ignoring rendition with unknown container: ${config.container.kind}`); + // `kind` is the literal "unknown" tag; the container the publisher actually named is in `raw`. + const kind = config.container.kind === "unknown" ? config.container.raw.kind : config.container.kind; + console.warn(`video: ignoring rendition with unknown container: ${kind}`); return false; } diff --git a/rs/CLAUDE.md b/rs/CLAUDE.md index d7bd26129e..8569f4a84b 100644 --- a/rs/CLAUDE.md +++ b/rs/CLAUDE.md @@ -40,7 +40,7 @@ Layered roughly transport -> container/format -> media -> apps/bindings. - `moq-hls` (lib): HLS / LL-HLS gateway (import + export, playlists + fMP4 via `moq-mux`). - `moq-bench` (bin): relay load generator. `JoinSet`-spawned staggered connections, rand sampling. - `moq-boy` (bin): crowd-controlled Game Boy emulator publisher (blocking emulator thread + async monitor tasks). -- `moq-token` (lib) / `moq-token` (bin from the `moq-token-cli` crate): JWT auth. `Claims`, `Algorithm`, `KeyType` (EC/RSA/OCT/OKP), JWKS. CLI does generate/sign/verify. +- `moq-token` (lib) / `moq-token` (bin from the `moq-token-cli` crate): JWT auth. `Claims`, `Algorithm`, `KeyMaterial` (EC/RSA/OCT/OKP), JWKS. CLI does generate/sign/verify. **Bindings** diff --git a/rs/moq-token/src/key.rs b/rs/moq-token/src/key.rs index 42e5f6c113..3aa301adba 100644 --- a/rs/moq-token/src/key.rs +++ b/rs/moq-token/src/key.rs @@ -143,7 +143,8 @@ pub struct RsaAdditionalPrime { /// because it's annoying to implement. /// /// This is the serialized form of a key, with plain fields you can build and edit. It is not -/// usable on its own: convert it into a [`Key`] via `Key::try_from` to sign or verify anything. +/// usable on its own: call [`validate`](Self::validate) (or the equivalent `Key::try_from`) to get +/// a [`Key`] that can sign and verify. #[derive(Clone, Serialize, Deserialize)] #[serde(remote = "Self")] #[non_exhaustive] From f7fdeb771f82ef27c740255015b3775fb0edc19c Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 22 Jul 2026 09:31:04 -0700 Subject: [PATCH 10/11] refactor(token)!: name the Jwk <-> Key conversion import/export `Jwk::validate` collided with `Scope::validate` and `Claims::validate`, which check and return nothing rather than converting. It called `scope.validate()?` two lines into its own body, so one verb meant two things in adjacent lines. Renames it to `Jwk::import` and adds the matching `Key::export`, borrowing the verb WebCrypto uses for this exact operation (`importKey`) and that `js/token` already uses on the other side of this crate pair (`importJoseKey`). The pair also gives the reverse direction a discoverable name instead of only an anonymous `From<&Key> for Jwk` impl, which now delegates to `export` just as `TryFrom` delegates to `import`. Co-Authored-By: Claude Opus 4.8 --- rs/moq-token/src/key.rs | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/rs/moq-token/src/key.rs b/rs/moq-token/src/key.rs index 3aa301adba..b295657e90 100644 --- a/rs/moq-token/src/key.rs +++ b/rs/moq-token/src/key.rs @@ -143,8 +143,8 @@ pub struct RsaAdditionalPrime { /// because it's annoying to implement. /// /// This is the serialized form of a key, with plain fields you can build and edit. It is not -/// usable on its own: call [`validate`](Self::validate) (or the equivalent `Key::try_from`) to get -/// a [`Key`] that can sign and verify. +/// usable on its own: call [`import`](Self::import) to get a [`Key`] that can sign and verify, +/// and [`Key::export`] to go back the other way. #[derive(Clone, Serialize, Deserialize)] #[serde(remote = "Self")] #[non_exhaustive] @@ -185,11 +185,12 @@ impl Jwk { } } - /// Check the parameters and turn this into a [`Key`] that can sign and verify. + /// Check the parameters and import this as a [`Key`] that can sign and verify. /// - /// The named counterpart to `Key::try_from`, so the conversion is discoverable from here - /// rather than only from a trait impl. - pub fn validate(self) -> crate::Result { + /// The inverse of [`Key::export`]. Named rather than only a `TryFrom` impl so the conversion + /// is discoverable from here, and `import`/`export` rather than `validate` because the + /// `validate` methods elsewhere in this crate check without converting. + pub fn import(self) -> crate::Result { if let Some(scope) = &self.scope { scope.validate()?; } @@ -262,13 +263,13 @@ impl TryFrom for Key { type Error = crate::Error; fn try_from(jwk: Jwk) -> crate::Result { - jwk.validate() + jwk.import() } } impl From<&Key> for Jwk { fn from(key: &Key) -> Self { - key.jwk.clone() + key.export() } } @@ -305,6 +306,14 @@ impl fmt::Debug for Key { } impl Key { + /// The serializable [`Jwk`] behind this key, cloned so editing it can't reach the original. + /// + /// The inverse of [`Jwk::import`]. Use it to derive a variant: export, edit, import again. + /// Reading a single field needs no clone, since a [`Key`] derefs to its [`Jwk`]. + pub fn export(&self) -> Jwk { + self.jwk.clone() + } + /// Parse a key from a string, auto-detecting JSON or base64url encoding. #[allow(clippy::should_implement_trait)] pub fn from_str(s: &str) -> crate::Result { @@ -674,7 +683,7 @@ mod tests { }, ); jwk.kid = Some(crate::KeyId::decode("test-key-1").unwrap()); - jwk.validate().unwrap() + jwk.import().unwrap() } fn create_test_claims() -> Claims { From e392a3b924590fd170b004688b02a2ac4ab0e9c3 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Wed, 22 Jul 2026 09:52:09 -0700 Subject: [PATCH 11/11] test(mux): guard the stranded-rendition test against a vacuous pass The test squats a timeline track name so building the media producer fails, then asserts no rendition was published. If the fixture ever stopped reaching track import, that assertion would pass for the wrong reason. Adds a control that runs the same fixture without the collision and requires it to publish exactly one rendition, so the failure path is provably the thing being measured. Asserting on `decode`'s result instead, as the review suggested, would not work: the importer logs and skips a track it cannot build rather than failing the whole decode, so `decode` returns `Ok`. Verified by trying it. Also corrects two docs. Both `closed()` methods on the broadcast still said closure happens when every producer drops, which stopped being the whole story when `finish` began closing the broadcast itself. And `Jwk::import` claimed to produce a key that "can sign and verify", when what the key may do is whatever `key_ops` allows: a verify-only public JWK imports fine and simply cannot sign. Co-Authored-By: Claude Opus 4.8 --- rs/moq-mux/src/container/mkv/import_test.rs | 6 ++++++ rs/moq-net/src/model/broadcast.rs | 6 ++++-- rs/moq-token/src/key.rs | 7 ++++--- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/rs/moq-mux/src/container/mkv/import_test.rs b/rs/moq-mux/src/container/mkv/import_test.rs index e121b4c2d7..221825f6ce 100644 --- a/rs/moq-mux/src/container/mkv/import_test.rs +++ b/rs/moq-mux/src/container/mkv/import_test.rs @@ -389,6 +389,10 @@ fn rendition_is_not_published_when_the_media_producer_fails() { .segment_end() .build(); + // Control: the same fixture publishes exactly one rendition when nothing collides, so the + // assertion below cannot pass merely because the fixture stopped reaching track import. + assert_eq!(run(&data).video.renditions.len(), 1, "fixture must publish a rendition"); + let mut broadcast = moq_net::broadcast::Info::new().produce(); let catalog = crate::catalog::Producer::new(&mut broadcast).unwrap(); @@ -399,6 +403,8 @@ fn rendition_is_not_published_when_the_media_producer_fails() { let mut mkv = crate::container::mkv::Import::new(broadcast, catalog.reserve()); let buf = bytes::BytesMut::from(&data[..]); + // The importer logs and skips a track it cannot build, rather than failing the whole + // decode, so the outcome shows up in the catalog rather than in this result. let _ = mkv.decode(&buf); assert!( diff --git a/rs/moq-net/src/model/broadcast.rs b/rs/moq-net/src/model/broadcast.rs index 693211b08e..6ffbc4056a 100644 --- a/rs/moq-net/src/model/broadcast.rs +++ b/rs/moq-net/src/model/broadcast.rs @@ -643,7 +643,8 @@ impl Dynamic { } } - /// Block until the broadcast is closed (every producer dropped), returning the cause. + /// Block until the broadcast is closed, by [`Producer::finish`] or by every producer + /// dropping, returning the cause. pub async fn closed(&self) -> Error { kio::wait(|waiter| self.poll_closed(waiter)).await } @@ -838,7 +839,8 @@ impl Consumer { } } - /// Block until the broadcast is closed (every producer dropped) and return the cause. + /// Block until the broadcast is closed, by [`Producer::finish`] or by every producer + /// dropping, and return the cause. /// /// Always returns [`Error::Dropped`]: a broadcast is just a collection of tracks, so it /// only ends when every producer is gone. There is no way to abort it with a code. diff --git a/rs/moq-token/src/key.rs b/rs/moq-token/src/key.rs index b295657e90..550fb07f66 100644 --- a/rs/moq-token/src/key.rs +++ b/rs/moq-token/src/key.rs @@ -143,8 +143,9 @@ pub struct RsaAdditionalPrime { /// because it's annoying to implement. /// /// This is the serialized form of a key, with plain fields you can build and edit. It is not -/// usable on its own: call [`import`](Self::import) to get a [`Key`] that can sign and verify, -/// and [`Key::export`] to go back the other way. +/// usable on its own: call [`import`](Self::import) to validate it and get a usable [`Key`], and +/// [`Key::export`] to go back the other way. What that key may do is whatever `key_ops` allows, +/// so a verify-only JWK imports fine and simply cannot sign. #[derive(Clone, Serialize, Deserialize)] #[serde(remote = "Self")] #[non_exhaustive] @@ -185,7 +186,7 @@ impl Jwk { } } - /// Check the parameters and import this as a [`Key`] that can sign and verify. + /// Validate the parameters and import this as a usable [`Key`]. /// /// The inverse of [`Key::export`]. Named rather than only a `TryFrom` impl so the conversion /// is discoverable from here, and `import`/`export` rather than `validate` because the