diff --git a/quest/m1/README.md b/quest/m1/README.md index 45b20598427..0596b4d706f 100644 --- a/quest/m1/README.md +++ b/quest/m1/README.md @@ -105,7 +105,6 @@ blocks. The quests that gated m0 lines moved under them. - [TS Opus export refusals](/quest/m1/ts-opus-export-refusals.md) - the TS exporter refuses Opus heads its channel code cannot describe instead of mislabeling them - [TS Opus channel codes](/quest/m1/ts-opus-channel-codes.md) - TS Opus with a channel code of 0x81 or above imports with its real head or is refused, never guessed as stereo - [AudioToolbox encode](/quest/m1/audio-encode-audiotoolbox.md) - macOS and iOS encode AAC-LC -- [AAC encode refusals](/quest/m1/aac-encode-refusals.md) - `Config::encode` refuses channel counts it cannot name instead of writing stereo - [GStreamer surround Opus](/quest/m1/gst-opus-surround.md) - the moq-gst sink publishes 3 to 8 channel Opus with the OpusHead its caps describe - [TS AAC PCE joins](/quest/m1/ts-aac-pce-join.md) - PCE-described AAC over TS plays after a mid-stream join or resume, and a missing PCE silences only its track - [FFI frame duration default](/quest/m1/ffi-frame-duration-default.md) - the binding audio encoder takes the codec's own frame by default, so `aac()` needs no explicit 0 diff --git a/quest/m1/aac-encode-refusals.md b/quest/m1/aac-encode-refusals.md deleted file mode 100644 index 18f97da5a28..00000000000 --- a/quest/m1/aac-encode-refusals.md +++ /dev/null @@ -1,27 +0,0 @@ -# [S] AAC config encode refuses unnameable channel counts - -## Goal - -`moq_mux::codec::aac::Config::encode` never writes an AudioSpecificConfig that -names the wrong layout. A channel count no channelConfiguration names is -refused, not logged and written as stereo, the way the ADTS writer refuses it. - -## Plan - -- `encode` returns `Bytes` today, so refusing is a published API break. - Decide whether it returns a `Result` or whether a - constructor validates the count up front so encoding cannot fail. -- A count a program config element could describe (7, or more than 8) is - still refused unless the caller supplies a layout; guessing speaker - positions is what this removes. -- Callers in `moq-audio` (encoder config, description synthesis) already - validate first; check they keep their error messages. `Producer` synthesizes - the ASC at construction, so it refuses such a layout there. -- JS already matches: `@moq/hang`'s `audioSpecificConfig` throws for the same - counts (#4119). Test every count from 1 to 8 and one beyond. - -Public API: `Config::encode` (or its constructor) changes. Wire: none. - -## Related - -- [#4283](https://github.com/moq-dev/moq/pull/4283) - the ADTS export refusals this mirrors diff --git a/rs/moq-audio/src/aac.rs b/rs/moq-audio/src/aac.rs index 52fb66e3dd3..2e65fdb73ce 100644 --- a/rs/moq-audio/src/aac.rs +++ b/rs/moq-audio/src/aac.rs @@ -78,14 +78,13 @@ pub(crate) fn description(catalog: &hang::catalog::AudioConfig, profile: u8) -> channel_count: catalog.channel_count, }; - // Synthesis is lossy in every field: the encoder masks the object type to - // the five bits it has, rewrites a channel count the config table can't - // name, and drops the sample rate's bits past 24. A rendition we can't - // decode would come back out of it looking like decodable stereo, so - // check the catalog's own numbers before encoding them. + // Synthesis still drops bits the config cannot hold: the object type is + // masked to five bits, and the sample rate to 24. Encode refuses a channel + // count that does not pick a channelConfiguration, but check the catalog + // first so the error stays the decoder's. validate(&config)?; - config.encode() + config.encode().map_err(moq_mux::Error::from)? } }; @@ -239,15 +238,14 @@ mod tests { #[test] fn rejects_more_than_stereo() { - // Synthesis would rewrite an unnameable count to stereo; 6 is nameable but - // still past what the decoder opens. + // 12 has no channelConfiguration; 6 does, but the decoder only opens mono and stereo. assert!(matches!( description(&catalog(2, 48_000, 12), 2), - Err(Error::Unsupported(_)) + Err(Error::Unsupported(msg)) if msg.contains("limited to mono and stereo") )); assert!(matches!( description(&catalog(2, 48_000, 6), 2), - Err(Error::Unsupported(_)) + Err(Error::Unsupported(msg)) if msg.contains("limited to mono and stereo") )); } diff --git a/rs/moq-audio/src/encode/encoder.rs b/rs/moq-audio/src/encode/encoder.rs index 8e704fb4249..78d173aacfc 100644 --- a/rs/moq-audio/src/encode/encoder.rs +++ b/rs/moq-audio/src/encode/encoder.rs @@ -386,7 +386,7 @@ impl Encoder { .map_err(moq_mux::Error::from)?; Some(head) } - Codec::Aac => Some(aac_config(settings)?.encode()), + Codec::Aac => Some(aac_config(settings)?.encode().map_err(moq_mux::Error::from)?), Codec::Pcm => None, }; @@ -1006,7 +1006,12 @@ mod tests { Layout::SixPointOne, Layout::Discrete(2), ] { - assert!(matches!(stub(&aac(layout)), Err(Error::Unsupported(_))), "{layout:?}"); + let result = stub(&aac(layout)); + let detail = result.as_ref().err().map(ToString::to_string); + assert!( + matches!(result, Err(Error::Unsupported(msg)) if msg.contains("no channelConfiguration")), + "{layout:?}: {detail:?}" + ); } } diff --git a/rs/moq-ffi/src/test.rs b/rs/moq-ffi/src/test.rs index 52646ff12f6..cdb4b1ed76c 100644 --- a/rs/moq-ffi/src/test.rs +++ b/rs/moq-ffi/src/test.rs @@ -1480,7 +1480,7 @@ async fn publish_media_aac_populates_description() { sample_rate: 44_100, channel_count: 2, }; - let init = config.encode(); + let init = config.encode().unwrap(); let _media = broadcast .publish_audio(audio_init(MoqAudioFormat::Aac, init.to_vec())) .unwrap(); diff --git a/rs/moq-mux/src/catalog/tracks.rs b/rs/moq-mux/src/catalog/tracks.rs index 5f304f229bf..e680f648491 100644 --- a/rs/moq-mux/src/catalog/tracks.rs +++ b/rs/moq-mux/src/catalog/tracks.rs @@ -895,7 +895,8 @@ mod tests { sample_rate: 48_000, channel_count: 2, } - .into(); + .try_into() + .unwrap(); config.jitter = Some(Duration::ZERO); assert!(crate::codec::aac::Import::new(track, reserved, config).is_err()); assert!(catalog.snapshot().audio.renditions.is_empty()); diff --git a/rs/moq-mux/src/codec/aac/import.rs b/rs/moq-mux/src/codec/aac/import.rs index c5f6571b64f..bc5ce7f2dd3 100644 --- a/rs/moq-mux/src/codec/aac/import.rs +++ b/rs/moq-mux/src/codec/aac/import.rs @@ -20,8 +20,9 @@ impl Import { /// Publish on an existing track producer with a resolved catalog config. /// /// Build one from an AudioSpecificConfig with [`config`] (which keeps its bytes as the catalog - /// `description`), or from an out-of-band [`Config`] via `into()` (which synthesizes the - /// description). The rendition publishes immediately. + /// `description`), or from an out-of-band [`Config`] via `try_into()` (which synthesizes the + /// description, refusing a channel count that does not pick a channelConfiguration). The rendition + /// publishes immediately. pub fn new( track: moq_net::track::Producer, reserved: crate::catalog::Reserved, @@ -117,24 +118,37 @@ impl Import { /// Errors on a malformed or empty buffer. pub fn config(init: &[u8]) -> crate::Result { let mut buf = init; - let mut audio: hang::catalog::AudioConfig = Config::parse(&mut buf)?.into(); + let parsed = Config::parse(&mut buf)?; + // Keep the bytes verbatim. Re-encoding would refuse a program config element whose count does + // not pick a channelConfiguration, and would drop any SBR or PS extension the parse ignores. + let mut audio = catalog_config(&parsed); audio.description = Some(bytes::Bytes::copy_from_slice(init)); Ok(audio) } -impl From for hang::catalog::AudioConfig { +impl TryFrom for hang::catalog::AudioConfig { + type Error = super::Error; + /// Build a catalog config from a config resolved out of band (an ADTS header, gstreamer caps), /// synthesizing the AudioSpecificConfig `description` since no verbatim bytes are available. - fn from(config: Config) -> Self { - let mut audio = hang::catalog::AudioConfig::new( - hang::catalog::AAC { - profile: config.profile, - }, - config.sample_rate, - config.channel_count, - ); - audio.container = hang::catalog::Container::Legacy; - audio.description = Some(config.encode()); - audio + /// + /// Refuses a channel count that does not pick a channelConfiguration, rather than describing it as + /// another layout. + fn try_from(config: Config) -> Result { + let mut audio = catalog_config(&config); + audio.description = Some(config.encode()?); + Ok(audio) } } + +fn catalog_config(config: &Config) -> hang::catalog::AudioConfig { + let mut audio = hang::catalog::AudioConfig::new( + hang::catalog::AAC { + profile: config.profile, + }, + config.sample_rate, + config.channel_count, + ); + audio.container = hang::catalog::Container::Legacy; + audio +} diff --git a/rs/moq-mux/src/codec/aac/mod.rs b/rs/moq-mux/src/codec/aac/mod.rs index ad46c1773b9..8707da1872c 100644 --- a/rs/moq-mux/src/codec/aac/mod.rs +++ b/rs/moq-mux/src/codec/aac/mod.rs @@ -32,6 +32,15 @@ pub enum Error { #[error("reserved channelConfiguration: {0}")] ReservedChannelConfig(u8), + /// A channel count that does not pick a channelConfiguration on its own. + /// + /// Counts 1 through 6 are that configuration, and 8 channels are configuration 7 (7.1). + /// Seven channels (6.1, or configuration 11) and 24 (22.2, configuration 13) have a + /// configuration but also program config element layouts, so the speaker positions are + /// not guessed. Every other count has no configuration at all. + #[error("cannot pick a channelConfiguration for {0} channels")] + UnsupportedChannelCount(u32), + /// A program config element is used with an object type that has no GASpecificConfig. #[error("channelConfiguration 0 is unsupported for audioObjectType {0}")] ProgramConfigUnsupported(u8), @@ -97,8 +106,9 @@ impl Config { /// Encode this configuration as an AudioSpecificConfig (ISO 14496-3 §1.6.2.1). /// /// Standard sample rates produce 2 bytes; non-standard rates fall back to - /// the 5-byte form with an explicit 24-bit frequency. - pub fn encode(&self) -> Bytes { + /// the 5-byte form with an explicit 24-bit frequency. A channel count that does + /// not pick a channelConfiguration is refused rather than written as another layout. + pub fn encode(&self) -> Result { // audioObjectType is a 5-bit field; mask to prevent shift overflow. let profile = self.profile & 0x1F; @@ -119,13 +129,13 @@ impl Config { _ => 0xF, // explicit 24-bit frequency follows }; - let channel_config = channel_config_from_count(self.channel_count) as u64; + let channel_config = channel_config_from_count(self.channel_count)? as u64; if freq_index != 0xF { // 5 + 4 + 4 = 13 bits → 2 bytes (3 bits padding) let b0 = (profile << 3) | (freq_index >> 1); let b1 = ((freq_index & 1) << 7) | ((channel_config as u8 & 0x0F) << 3); - Bytes::from(vec![b0, b1]) + Ok(Bytes::from(vec![b0, b1])) } else { // 5 + 4 + 24 + 4 = 37 bits → 5 bytes (3 bits padding) let mut bits: u64 = 0; @@ -134,7 +144,7 @@ impl Config { bits |= (self.sample_rate as u64) << 7; bits |= (channel_config & 0xF) << 3; let all = bits.to_be_bytes(); - Bytes::copy_from_slice(&all[3..8]) + Ok(Bytes::copy_from_slice(&all[3..8])) } } } @@ -431,16 +441,18 @@ fn channel_count_from_config(channel_config: u8) -> Result { } } -/// Inverse of [`channel_count_from_config`]. Defaults to stereo for unsupported -/// counts (channel configs > 7 are reserved). -fn channel_config_from_count(channel_count: u32) -> u8 { +/// Inverse of [`channel_count_from_config`] for the configurations a bare count names. +/// +/// Counts 1 through 6 are that configuration. Eight channels are configuration 7, the +/// 7.1 layout, so a configuration 12 or 14 source keeps its layout only through its +/// verbatim AudioSpecificConfig. Any other count is refused rather than guessing speaker +/// positions, including 7 and 24, which configurations 11 and 13 name alongside program +/// config element layouts. +fn channel_config_from_count(channel_count: u32) -> Result { match channel_count { - 1..=6 => channel_count as u8, - 8 => 7, - _ => { - tracing::warn!(channel_count, "unsupported channel count, defaulting to stereo"); - 2 - } + 1..=6 => Ok(channel_count as u8), + 8 => Ok(7), + _ => Err(Error::UnsupportedChannelCount(channel_count)), } } @@ -470,7 +482,7 @@ mod tests { sample_rate: 44_056, // not in the standard table channel_count: 2, }; - let encoded = cfg.encode(); + let encoded = cfg.encode().unwrap(); assert_eq!(encoded.len(), 5, "explicit-rate config is 5 bytes"); let parsed = Config::parse(&mut encoded.as_ref()).unwrap(); @@ -499,7 +511,7 @@ mod tests { sample_rate: 48000, channel_count: 6, }; - let encoded = cfg.encode(); + let encoded = cfg.encode().unwrap(); let parsed = Config::parse(&mut encoded.as_ref()).unwrap(); assert_eq!(parsed.channel_count, 6); } @@ -512,7 +524,7 @@ mod tests { sample_rate: 48000, channel_count: 8, }; - let encoded = cfg.encode(); + let encoded = cfg.encode().unwrap(); let parsed = Config::parse(&mut encoded.as_ref()).unwrap(); assert_eq!(parsed.channel_count, 8, "7.1 surround should round-trip as 8 channels"); } @@ -748,12 +760,66 @@ mod tests { sample_rate: 44_100, channel_count: 2, } - .encode(); + .encode() + .unwrap(); assert_eq!(asc, encoded); } #[test] - fn unsupported_channel_count_falls_back_to_stereo_config() { - assert_eq!(channel_config_from_count(9), 2); + fn encode_names_only_channel_configurations() { + // 48 kHz AAC-LC. Frequency index 3's low bit sits in the second byte, then the + // channelConfiguration, so that byte is 0x80 plus the configuration shifted by 3. + for count in 1..=8 { + let encoded = Config { + profile: 2, + sample_rate: 48_000, + channel_count: count, + } + .encode(); + let config = match count { + 1..=6 => count as u8, + 8 => 7, + _ => { + assert!(matches!(encoded, Err(Error::UnsupportedChannelCount(got)) if got == count)); + continue; + } + }; + let encoded = encoded.unwrap(); + assert_eq!(encoded.as_ref(), &[0x11, 0x80 | (config << 3)], "{count} channels"); + assert_eq!(Config::parse(&mut encoded.as_ref()).unwrap().channel_count, count); + } + + // One past the configurations a count names, plus zero and 22.2 (configuration 13). + for count in [0, 9, 24] { + assert!(matches!( + Config { + profile: 2, + sample_rate: 48_000, + channel_count: count, + } + .encode(), + Err(Error::UnsupportedChannelCount(got)) if got == count + )); + } + } + + #[test] + fn catalog_synthesis_refuses_an_unnameable_count() { + let err = hang::catalog::AudioConfig::try_from(Config { + profile: 2, + sample_rate: 48_000, + channel_count: 7, + }) + .unwrap_err(); + assert!(matches!(err, Error::UnsupportedChannelCount(7))); + } + + #[test] + fn verbatim_config_keeps_a_program_config_element() { + // Encode refuses seven channels. Import keeps the element instead of re-encoding. + let asc = pce_asc(2, &[false, true], &[true], &[true], 0); + let audio = config(&asc).unwrap(); + assert_eq!(audio.channel_count, 7); + assert_eq!(audio.description.as_deref(), Some(asc.as_slice())); } } diff --git a/rs/moq-mux/src/container/fmp4/export_test.rs b/rs/moq-mux/src/container/fmp4/export_test.rs index dbefeb16f26..56f942ea5dc 100644 --- a/rs/moq-mux/src/container/fmp4/export_test.rs +++ b/rs/moq-mux/src/container/fmp4/export_test.rs @@ -84,7 +84,8 @@ async fn legacy_aac_source_to_cmaf_export_synthesizes_esds() { sample_rate: 44100, channel_count: 2, } - .encode(); + .encode() + .unwrap(); let mut config = AudioConfig::new(AAC { profile: 2 }, 44100, 2); config.description = Some(description); @@ -700,7 +701,7 @@ async fn one_packet_audio_group_is_timed_by_the_catalog() { channel_count: 2, }; let mut config = AudioConfig::new(AAC { profile: 2 }, 44100, 2); - config.description = Some(aac.encode()); + config.description = Some(aac.encode().unwrap()); let mut live = Live::audio(config); live.track.write(raw_frame(0, &[0x01, 0x02, 0x03, 0x04], true)).unwrap(); @@ -1634,7 +1635,8 @@ async fn aac_bitrate_churn_keeps_the_track() { sample_rate: 44100, channel_count: 2, } - .encode(), + .encode() + .unwrap(), ); let mut live = Live::audio(config); let name = live.track.name().to_string(); diff --git a/rs/moq-mux/src/import/track.rs b/rs/moq-mux/src/import/track.rs index 49971acc583..7a440c93507 100644 --- a/rs/moq-mux/src/import/track.rs +++ b/rs/moq-mux/src/import/track.rs @@ -911,7 +911,7 @@ mod tests { sample_rate: 44_100, channel_count: 2, }; - let init = config.encode(); + let init = config.encode().unwrap(); let request = broadcast.reserve_track("audio").unwrap(); let import = Track::audio(