From dea968f32b107b0ffafc6cc32e17720036016f35 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Thu, 30 Jul 2026 10:09:01 -0700 Subject: [PATCH] Stop AudioChannels and SampleRate fabricating a value for Unknown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three copies of the same two mappings existed. The canonical accessors returned 6 channels and 48000 Hz for Unknown; a third copy in diag.rs returned 0. The honest one was the copy. A plausible wrong answer is worse than an obvious one. Six channels at 48 kHz is indistinguishable from a real 5.1 track, so every caller became responsible for remembering to check the variant first — and this crate walked into exactly that: the json:// sink reported a confident 5.1 for audio whose neighbouring fields said "unknown". That was fixed at the call site earlier in this audit; this fixes it at the source. The accessors now return 0, which is what both in-crate call sites already coerced Unknown to by hand, so their guards are gone and the behaviour is unchanged. Zero is also obviously wrong if it ever reaches output, where six is not. The diag.rs duplicates are deleted rather than corrected — a fourth copy would have drifted too. Their only caller was a trace line in the same file, now on the canonical accessors. Their tests moved across and gained the Unknown case, which is the point: restoring either fabricated value fails both. Found by the round-7 correctness agent while fixing the json:// sink; it flagged the third copy as out of its scope rather than touching it. --- src/diag.rs | 80 ++++++++++++++++++++++--------------------------- src/disc/mod.rs | 13 ++++++-- src/mux/mkv.rs | 18 ++++------- 3 files changed, 53 insertions(+), 58 deletions(-) diff --git a/src/diag.rs b/src/diag.rs index 57ef041..65395df 100644 --- a/src/diag.rs +++ b/src/diag.rs @@ -22,10 +22,7 @@ //! `Disc`-level dump ([`dump_disc`]) covers everything that survives //! lowering: titles, streams, the picked main feature, and AACS state. -use crate::disc::{ - AudioChannels, ColorSpace, Disc, DiscTitle, FrameRate, HdrFormat, Resolution, SampleRate, - Stream, -}; +use crate::disc::{ColorSpace, Disc, DiscTitle, FrameRate, HdrFormat, Resolution, Stream}; use crate::ifo::{CellCategory, DvdTitle}; const DIAG: &str = "freemkv::diag"; @@ -95,36 +92,12 @@ pub fn hdr_str(h: HdrFormat) -> &'static str { } } -/// Channel count from an [`AudioChannels`] layout (what lands in the MKV -/// `Channels` element). -pub fn channel_count(ch: AudioChannels) -> u8 { - match ch { - AudioChannels::Mono => 1, - AudioChannels::Stereo => 2, - AudioChannels::Stereo21 => 3, - AudioChannels::Quad => 4, - AudioChannels::Surround50 => 5, - AudioChannels::Surround51 => 6, - AudioChannels::Surround61 => 7, - AudioChannels::Surround71 => 8, - AudioChannels::Unknown => 0, - } -} - -/// Sample-rate in Hz for a [`SampleRate`]. -pub fn sample_rate_hz(s: SampleRate) -> u32 { - match s { - SampleRate::S44_1 => 44100, - SampleRate::S48 => 48000, - SampleRate::S88_2 => 88200, - SampleRate::S96 => 96000, - SampleRate::S176_4 => 176400, - SampleRate::S192 => 192000, - SampleRate::S48_96 => 96000, - SampleRate::S48_192 => 192000, - SampleRate::Unknown => 0, - } -} +// `channel_count` and `sample_rate_hz` lived here as a third copy of the +// AudioChannels/SampleRate mappings. They were the only HONEST copy — returning +// 0 for Unknown where the canonical accessors fabricated 6 channels at 48 kHz — +// and their only caller was the trace line below, in this same file. The +// canonical accessors are honest now, so the duplicates are gone rather than +// left to drift a fourth time. // ── DVD cell-category dump (from the IFO scan, pre-lowering) ───────────────── @@ -610,8 +583,8 @@ fn dump_title(ti: usize, title: &DiscTitle) { a.pid, a.codec, a.channels, - channel_count(a.channels), - sample_rate_hz(a.sample_rate), + a.channels.count(), + a.sample_rate.hz(), a.language, a.secondary, ), @@ -631,6 +604,10 @@ fn dump_title(ti: usize, title: &DiscTitle) { #[cfg(test)] mod tests { use super::*; + // Needed only by the tests: the production code in this file no longer names + // these types directly, since the local channel/sample-rate duplicates were + // deleted in favour of the canonical accessors. + use crate::disc::{AudioChannels, SampleRate}; #[test] fn res_str_keeps_interlace_marker() { @@ -656,18 +633,33 @@ mod tests { assert_eq!(hdr_str(HdrFormat::Sdr), "SDR"); } + /// Moved from the deleted local duplicates onto the canonical accessors, + /// with the Unknown case added — which is the whole point of the change. #[test] - fn channel_count_matches_layout() { - assert_eq!(channel_count(AudioChannels::Mono), 1); - assert_eq!(channel_count(AudioChannels::Stereo), 2); - assert_eq!(channel_count(AudioChannels::Surround51), 6); - assert_eq!(channel_count(AudioChannels::Surround71), 8); + fn channel_count_matches_layout_and_is_zero_when_unknown() { + assert_eq!(AudioChannels::Mono.count(), 1); + assert_eq!(AudioChannels::Stereo.count(), 2); + assert_eq!(AudioChannels::Surround51.count(), 6); + assert_eq!(AudioChannels::Surround71.count(), 8); + // The one that matters. This used to return 6, which is indistinguishable + // from a real 5.1 track and left every caller responsible for checking + // the variant first. + assert_eq!( + AudioChannels::Unknown.count(), + 0, + "an unknown layout must not report a plausible channel count" + ); } #[test] - fn sample_rate_hz_values() { - assert_eq!(sample_rate_hz(SampleRate::S48), 48000); - assert_eq!(sample_rate_hz(SampleRate::S96), 96000); + fn sample_rate_hz_values_and_zero_when_unknown() { + assert_eq!(SampleRate::S48.hz(), 48000.0); + assert_eq!(SampleRate::S96.hz(), 96000.0); + assert_eq!( + SampleRate::Unknown.hz(), + 0.0, + "an unknown sample rate must not report a plausible 48 kHz" + ); } #[test] diff --git a/src/disc/mod.rs b/src/disc/mod.rs index 68f5097..9d1c0b5 100644 --- a/src/disc/mod.rs +++ b/src/disc/mod.rs @@ -1035,7 +1035,14 @@ impl AudioChannels { AudioChannels::Surround51 => 6, AudioChannels::Surround61 => 7, AudioChannels::Surround71 => 8, - AudioChannels::Unknown => 6, + // 0, not 6. An unknown layout has no channel count, and returning a + // plausible one made every caller responsible for remembering to + // check the variant first — a trap, and one this crate walked into: + // the json:// sink reported a confident 5.1 for audio its own + // neighbouring fields called "unknown". 0 is the value Matroska and + // the sinks already coerce Unknown to, and unlike 6 it is obviously + // wrong if it ever reaches output. + AudioChannels::Unknown => 0, } } @@ -1082,7 +1089,9 @@ impl SampleRate { SampleRate::S96 => 96000.0, SampleRate::S176_4 => 176400.0, SampleRate::S192 => 192000.0, - SampleRate::Unknown => 48000.0, + // 0.0, not 48000.0 — see AudioChannels::count. A fabricated rate is + // indistinguishable from a real one; a zero is not. + SampleRate::Unknown => 0.0, } } diff --git a/src/mux/mkv.rs b/src/mux/mkv.rs index f15c843..2ca49f2 100644 --- a/src/mux/mkv.rs +++ b/src/mux/mkv.rs @@ -7,8 +7,7 @@ use super::ebml; use super::timeline::TimelineContinuity; use crate::disc::{ - AudioChannels, AudioStream, Chapter, Codec, ColorSpace, HdrFormat, Resolution, SampleRate, - SubtitleStream, VideoStream, + AudioStream, Chapter, Codec, ColorSpace, HdrFormat, Resolution, SubtitleStream, VideoStream, }; use std::io::{self, Seek, Write}; @@ -515,16 +514,11 @@ impl MkvTrack { // the SamplingFrequency / Channels element (Matroska supplies its own // spec default) rather than writing a fabricated 48000 Hz / 6-channel // value into the file. - let sr = if matches!(a.sample_rate, SampleRate::Unknown) { - 0.0 - } else { - a.sample_rate.hz() - }; - let ch = if matches!(a.channels, AudioChannels::Unknown) { - 0 - } else { - a.channels.count() - }; + // No guard needed: the accessors return 0 for Unknown, which is exactly + // what this wants. They used to fabricate 6 channels at 48 kHz, so every + // caller had to remember to check the variant first. + let sr = a.sample_rate.hz(); + let ch = a.channels.count(); let name = a.label.clone();