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..074977007b 100644 --- a/doc/lib/rs/env/native.md +++ b/doc/lib/rs/env/native.md @@ -185,9 +185,12 @@ 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. +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..c3973b7e0e --- /dev/null +++ b/js/hang/src/catalog/container.test.ts @@ -0,0 +1,64 @@ +import { expect, test } from "bun:test"; +import { type Container, ContainerSchema, containerSupported } from "./container.ts"; +import { RootSchema } from "./root.ts"; + +test("known containers round-trip", () => { + 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); + } +}); + +test("unknown container is preserved instead of throwing", () => { + const container = { kind: "future", extra: { nested: [1, 2] }, flag: true }; + const parsed = ContainerSchema.parse(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); +}); + +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 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", () => { + // `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: "unknown", + raw: { kind: "future", magic: 7 }, + }); +}); diff --git a/js/hang/src/catalog/container.ts b/js/hang/src/catalog/container.ts index 13d7c64edc..be55f54ddf 100644 --- a/js/hang/src/catalog/container.ts +++ b/js/hang/src/catalog/container.ts @@ -1,5 +1,32 @@ 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. + * + * 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 + * fall through to this arm, still report as CMAF, and hand decoders an undefined init segment. + */ +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. * @@ -9,21 +36,41 @@ 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. */ +/** + * 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 a container can be decoded by this build, i.e. its `kind` is recognized. */ +export function containerSupported(container: Container): boolean { + return container.kind !== "unknown"; +} diff --git a/js/watch/src/audio/decoder.ts b/js/watch/src/audio/decoder.ts index 1d52f3e96a..2f23468508 100644 --- a/js/watch/src/audio/decoder.ts +++ b/js/watch/src/audio/decoder.ts @@ -513,6 +513,13 @@ export class Decoder { } async function supported(config: Catalog.AudioConfig): Promise { + if (!Catalog.containerSupported(config.container)) { + // `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; + } + // 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. diff --git a/js/watch/src/video/decoder.ts b/js/watch/src/video/decoder.ts index 3574a82b47..b820b409f8 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 (container.kind !== "cmaf") 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,6 +556,13 @@ class DecoderTrack { } async function supported(config: Catalog.VideoConfig): Promise { + if (!Catalog.containerSupported(config.container)) { + // `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; + } + let description: Uint8Array | undefined; if (config.description) { description = Util.Hex.toBytes(config.description); 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/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..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(); @@ -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-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-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..0adad76901 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,50 @@ 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. + catalog.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..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, }, @@ -407,19 +414,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 +441,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/mkv/import_test.rs b/rs/moq-mux/src/container/mkv/import_test.rs index a3f94aa0f4..221825f6ce 100644 --- a/rs/moq-mux/src/container/mkv/import_test.rs +++ b/rs/moq-mux/src/container/mkv/import_test.rs @@ -373,3 +373,42 @@ 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(); + + // 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(); + + // 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[..]); + // 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!( + catalog.snapshot().video.renditions.is_empty(), + "a rendition whose media producer failed must not be advertised" + ); +} 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..12e1259167 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()); } } @@ -643,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 { @@ -651,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, @@ -660,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. @@ -670,6 +678,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 +699,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 +720,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 +766,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 +779,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 +801,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 +820,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 +850,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 +1099,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 +1168,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 +1246,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 +1303,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 +1448,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 +1506,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 +1879,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 +1891,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 +1936,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 +1947,10 @@ mod test { "upgrade advertises the cue track" ); + // 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(); 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..23bfa05ee1 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, } @@ -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,6 +350,46 @@ 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 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. + 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(); 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 e15cd35d0a..6ffbc4056a 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(); } } @@ -636,13 +643,14 @@ 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 } /// 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) } @@ -831,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. @@ -1214,7 +1223,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..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()); @@ -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); @@ -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-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-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/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..d2008bf2f4 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,37 @@ 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()); + // 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(()) } } 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 +64,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..7560220c34 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,37 @@ 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()); + // 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(()) } } 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 +62,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 +91,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/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-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..df7c253e8a 100644 --- a/rs/moq-rtmp/src/dial.rs +++ b/rs/moq-rtmp/src/dial.rs @@ -475,13 +475,15 @@ impl Publisher { fn finish(&mut self) -> anyhow::Result<()> { self.importer.finish()?; - self.broadcast.clone().finish(); + self.broadcast.finish(); Ok(()) } /// 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..41b3ff44ea 100644 --- a/rs/moq-rtmp/src/server.rs +++ b/rs/moq-rtmp/src/server.rs @@ -1205,14 +1205,16 @@ 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(()) } /// 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..8002cafd3b 100644 --- a/rs/moq-srt/src/ts.rs +++ b/rs/moq-srt/src/ts.rs @@ -57,13 +57,15 @@ 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(()) } /// 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-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(); } } diff --git a/rs/moq-token/src/generate.rs b/rs/moq-token/src/generate.rs index 8c7b692949..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, Key, KeyOperation, 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; @@ -21,21 +21,15 @@ pub fn generate(algorithm: Algorithm, id: Option) -> crate::Result Algorithm::EdDSA => generate_ed25519_key(), }; - Ok(Key { - kid: id, - operations: [KeyOperation::Sign, KeyOperation::Verify].into(), - algorithm, - key: key?, - scope: None, - decode: Default::default(), - encode: Default::default(), - }) + let mut jwk = Jwk::new(algorithm, key?); + jwk.kid = id; + 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; @@ -64,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(), @@ -86,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, @@ -107,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, @@ -115,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 8fcae74668..550fb07f66 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")] @@ -141,9 +141,15 @@ 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: 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")] -pub struct Key { +#[non_exhaustive] +pub struct Jwk { /// The algorithm used by the key. #[serde(rename = "alg")] pub algorithm: Algorithm, @@ -152,27 +158,53 @@ pub struct Key { #[serde(rename = "key_ops")] pub operations: HashSet, - /// Defaults to KeyType::OCT + /// 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")] 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, +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, material: KeyMaterial) -> Self { + Self { + algorithm, + operations: [KeyOperation::Sign, KeyOperation::Verify].into(), + material, + kid: None, + scope: None, + } + } - #[serde(skip)] - pub(crate) encode: OnceLock, + /// 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 + /// `validate` methods elsewhere in this crate check without converting. + pub fn import(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 Key { +impl<'de> Deserialize<'de> for Jwk { fn deserialize(deserializer: D) -> Result where D: Deserializer<'de>, @@ -192,7 +224,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 +233,68 @@ 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 { + 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; + + fn try_from(jwk: Jwk) -> crate::Result { + jwk.import() + } +} + +impl From<&Key> for Jwk { + fn from(key: &Key) -> Self { + key.export() + } +} + +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 +307,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 { @@ -252,29 +354,33 @@ 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()); } - 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, @@ -282,11 +388,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(), + material, + kid: self.kid.clone(), + scope: self.scope.clone(), + }, decode: Default::default(), encode: Default::default(), }) @@ -297,12 +405,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, @@ -336,7 +444,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()); @@ -348,7 +456,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()) } }; @@ -361,12 +469,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 { @@ -383,7 +491,7 @@ impl Key { _ => return Err(KeyError::InvalidCurve("EC").into()), } } - KeyType::OKP { + KeyMaterial::OKP { ref curve, ref d, ref x, @@ -398,7 +506,7 @@ impl Key { _ => return Err(KeyError::InvalidCurve("OKP").into()), } } - KeyType::RSA { + KeyMaterial::RSA { ref public, ref private, } => { @@ -483,13 +591,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); + 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.jwk.operations = operations.into_iter().collect(); + self + } + fn validate_scope(&self, claims: &Claims) -> crate::Result<()> { if let Some(scope) = &self.scope { scope.validate()?; @@ -560,17 +677,14 @@ mod tests { use std::time::{Duration, SystemTime}; fn create_test_key() -> Key { - Key { - 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, - decode: Default::default(), - encode: Default::default(), - } + ); + jwk.kid = Some(crate::KeyId::decode("test-key-1").unwrap()); + jwk.import().unwrap() } fn create_test_claims() -> Claims { @@ -591,8 +705,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"), @@ -609,7 +723,7 @@ mod tests { assert!(key.is_ok()); let key = key.unwrap(); - if let KeyType::OCT { ref 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 { @@ -623,7 +737,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] @@ -687,10 +801,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 +900,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()); @@ -823,8 +989,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 { ref secret } => assert_eq!(secret.len(), 32), + match &key.material { + KeyMaterial::OCT { secret } => assert_eq!(secret.len(), 32), _ => panic!("Expected OCT key"), } } @@ -837,8 +1003,8 @@ mod tests { assert_eq!(key.algorithm, Algorithm::HS384); - match key.key { - KeyType::OCT { ref secret } => assert_eq!(secret.len(), 48), + match &key.material { + KeyMaterial::OCT { secret } => assert_eq!(secret.len(), 48), _ => panic!("Expected OCT key"), } } @@ -851,8 +1017,8 @@ mod tests { assert_eq!(key.algorithm, Algorithm::HS512); - match key.key { - KeyType::OCT { ref secret } => assert_eq!(secret.len(), 64), + match &key.material { + KeyMaterial::OCT { secret } => assert_eq!(secret.len(), 64), _ => panic!("Expected OCT key"), } } @@ -864,12 +1030,9 @@ mod tests { 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!(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); @@ -885,7 +1048,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] @@ -895,7 +1058,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] @@ -905,7 +1068,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] @@ -938,12 +1101,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 { @@ -965,17 +1128,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); @@ -999,16 +1162,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); @@ -1085,13 +1248,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 { @@ -1109,11 +1272,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 { @@ -1247,19 +1410,16 @@ mod tests { 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.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 { - public: ref guest_public, - private: ref public_private, + match &public_key.material { + KeyMaterial::RSA { + public: guest_public, + private: public_private, } => { assert!(public_private.is_none()); assert_eq!(public.n, guest_public.n); @@ -1285,19 +1445,16 @@ mod tests { 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.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 { - public: ref guest_public, - private: ref public_private, + match &public_key.material { + KeyMaterial::RSA { + public: guest_public, + private: public_private, } => { assert!(public_private.is_none()); assert_eq!(public.n, guest_public.n); @@ -1329,9 +1486,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 { @@ -1349,7 +1506,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"); @@ -1366,7 +1523,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"); @@ -1429,7 +1586,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); @@ -1498,11 +1655,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 { @@ -1533,11 +1690,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 { diff --git a/rs/moq-token/src/set.rs b/rs/moq-token/src/set.rs index 72ce969b19..5416dad895 100644 --- a/rs/moq-token/src/set.rs +++ b/rs/moq-token/src/set.rs @@ -77,10 +77,15 @@ 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.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() } @@ -166,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.as_deref(), Some("1")); + assert_eq!(set.keys[0].kid.as_ref().map(|k| k.encode()), Some("1")); assert!(set.find_key("1").is_some()); } @@ -220,7 +225,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.as_ref().map(|k| k.encode()), Some("my-key")); } #[test] @@ -247,11 +252,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 +261,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.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.as_deref(), Some("verify")); + assert_eq!(found_verify.unwrap().kid.as_ref().map(|k| k.encode()), Some("verify")); } #[test] @@ -279,7 +281,7 @@ 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_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)); } @@ -309,8 +311,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 +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.as_deref(), Some("1")); + assert_eq!(loaded.keys[0].kid.as_ref().map(|k| k.encode()), Some("1")); // Clean up let _ = std::fs::remove_file(path); 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), }