labels: fresh-eyes audit — capture variant, dedupe detect, lock registry
Three targeted fixes from a second-pass audit of the labels module. 1. vocab::lang now returns Option<LangInfo> with both code AND a human-readable variant string. Pre-fix: 'Brazilian Portuguese 5.1' became language=por, variant='', dropping the dialect info the disc had explicitly authored. Post-fix: language=por, variant='Brazilian' — matches the convention pixelogic / ctrm / criterion already use for their region variants. dbp now populates StreamLabel::variant from this. Compound table grew a 3-tuple (needle, code, variant); bare matches still return variant=''. 2. dbp and deluxe had duplicated detect() boilerplate (any top-level .jar in /BDMV/JAR/). Both now call jar::has_any_top_level_jar. The trait-level detect contract — see super::PARSERS — can't peek inside a jar without a SectorReader, so loose-detect-plus-real- check-in-parse is the unavoidable pattern for jar-content parsers. Consolidating in jar.rs at least makes the duplication visible. 3. mod.rs comment about parser ordering said 'dbp last'; deluxe is actually now last. Updated to explain the dbp-before-deluxe order is by cost (cp-iteration cheaper than bytecode walking when Phase D lands). Plus a registry-level lock test in mod.rs::registry_tests — asserts the PARSERS array order is exactly [paramount, criterion, pixelogic, ctrm, dbp, deluxe]. This was previously implicit; if someone reorders the array (which changes which parser wins on overlapping signals), unit tests would have stayed green. Now they fail with an explanatory message about why the order matters. Audit findings deferred to follow-ups (each its own commit + design discussion): - Stronger detect contract — current loose-detect-real-check pattern is forced by SectorReader-not-in-detect-signature; could be fixed by changing the trait to take an Option<&mut dyn SectorReader> or similar. - Per-parser confidence scoring — registry currently first-match-wins. A high-confidence parser ought to beat a low-confidence one regardless of array order. - class_reader fuzzing — handles malformed input via Result but no adversarial corpus yet. Precommit (cargo +1.86 fmt + clippy + test) green.
This commit is contained in:
+27
-25
@@ -46,13 +46,7 @@ use std::collections::BTreeMap;
|
||||
/// returns None on a mismatch — so this parser only ever consumes
|
||||
/// time on discs that fell through every earlier parser.
|
||||
pub fn detect(udf: &UdfFs) -> bool {
|
||||
let Some(jar_dir) = udf.find_dir("/BDMV/JAR") else {
|
||||
return false;
|
||||
};
|
||||
jar_dir
|
||||
.entries
|
||||
.iter()
|
||||
.any(|e| !e.is_dir && e.name.to_lowercase().ends_with(".jar"))
|
||||
jar::has_any_top_level_jar(udf)
|
||||
}
|
||||
|
||||
pub fn parse(reader: &mut dyn SectorReader, udf: &UdfFs) -> Option<Vec<StreamLabel>> {
|
||||
@@ -129,7 +123,9 @@ fn collect_textfield(
|
||||
}
|
||||
|
||||
fn make_label(num: u16, label: String, stream_type: StreamLabelType) -> StreamLabel {
|
||||
let language = vocab::lang(&label).unwrap_or_default().to_string();
|
||||
let lang_info = vocab::lang(&label);
|
||||
let language = lang_info.map(|l| l.code).unwrap_or("").to_string();
|
||||
let variant = lang_info.map(|l| l.variant).unwrap_or("").to_string();
|
||||
let qualifier = vocab::qualifier(&label);
|
||||
let purpose = vocab::purpose(&label);
|
||||
StreamLabel {
|
||||
@@ -140,7 +136,7 @@ fn make_label(num: u16, label: String, stream_type: StreamLabelType) -> StreamLa
|
||||
purpose,
|
||||
qualifier,
|
||||
codec_hint: String::new(),
|
||||
variant: String::new(),
|
||||
variant,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -221,31 +217,37 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn make_label_compound_languages() {
|
||||
assert_eq!(
|
||||
make_label(1, "Brazilian Portuguese 5.1".into(), StreamLabelType::Audio).language,
|
||||
"por"
|
||||
);
|
||||
assert_eq!(
|
||||
make_label(1, "Castilian Spanish".into(), StreamLabelType::Audio).language,
|
||||
"spa"
|
||||
);
|
||||
assert_eq!(
|
||||
make_label(
|
||||
fn make_label_compound_languages_populate_variant() {
|
||||
let brazilian = make_label(1, "Brazilian Portuguese 5.1".into(), StreamLabelType::Audio);
|
||||
assert_eq!(brazilian.language, "por");
|
||||
assert_eq!(brazilian.variant, "Brazilian");
|
||||
|
||||
let castilian = make_label(1, "Castilian Spanish".into(), StreamLabelType::Audio);
|
||||
assert_eq!(castilian.language, "spa");
|
||||
assert_eq!(castilian.variant, "Castilian");
|
||||
|
||||
let canadian = make_label(
|
||||
1,
|
||||
"Canadian French Dolby Digital".into(),
|
||||
StreamLabelType::Audio
|
||||
)
|
||||
.language,
|
||||
"fra"
|
||||
StreamLabelType::Audio,
|
||||
);
|
||||
assert_eq!(canadian.language, "fra");
|
||||
assert_eq!(canadian.variant, "Canadian");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn make_label_bare_language_has_empty_variant() {
|
||||
let l = make_label(1, "English Dolby Atmos".into(), StreamLabelType::Audio);
|
||||
assert_eq!(l.language, "eng");
|
||||
assert_eq!(l.variant, "");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn make_label_unknown_language_is_empty() {
|
||||
// vocab::lang returns None — make_label converts to "".
|
||||
// vocab::lang returns None — make_label converts both fields to "".
|
||||
let l = make_label(1, "Klingon Dolby Atmos".into(), StreamLabelType::Audio);
|
||||
assert_eq!(l.language, "");
|
||||
assert_eq!(l.variant, "");
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -62,13 +62,7 @@ pub fn detect(udf: &UdfFs) -> bool {
|
||||
// Cheap pre-check at the dir level; the real signal is
|
||||
// `com/bydeluxe/` inside any top-level jar's central directory,
|
||||
// which `parse()` confirms when given a `SectorReader`.
|
||||
let Some(jar_dir) = udf.find_dir("/BDMV/JAR") else {
|
||||
return false;
|
||||
};
|
||||
jar_dir
|
||||
.entries
|
||||
.iter()
|
||||
.any(|e| !e.is_dir && e.name.to_lowercase().ends_with(".jar"))
|
||||
jar::has_any_top_level_jar(udf)
|
||||
}
|
||||
|
||||
pub fn parse(reader: &mut dyn SectorReader, udf: &UdfFs) -> Option<Vec<StreamLabel>> {
|
||||
|
||||
@@ -23,6 +23,21 @@ use zip::ZipArchive;
|
||||
/// etc.
|
||||
pub type Jar = ZipArchive<Cursor<Vec<u8>>>;
|
||||
|
||||
/// True if `/BDMV/JAR/` contains at least one top-level `.jar` file
|
||||
/// (not under a subdir). Used by `detect()` in parsers whose real
|
||||
/// signal lives inside a jar — they can't open the jar without a
|
||||
/// `SectorReader`, so they use this cheap pre-check and do the real
|
||||
/// `com/<vendor>/` discriminator in `parse()`.
|
||||
pub fn has_any_top_level_jar(udf: &UdfFs) -> bool {
|
||||
let Some(jar_dir) = udf.find_dir("/BDMV/JAR") else {
|
||||
return false;
|
||||
};
|
||||
jar_dir
|
||||
.entries
|
||||
.iter()
|
||||
.any(|e| !e.is_dir && e.name.to_lowercase().ends_with(".jar"))
|
||||
}
|
||||
|
||||
/// Open every top-level `*.jar` entry in `/BDMV/JAR/` and yield each
|
||||
/// `(entry_name, Jar)` to `f`. Returns the first `Some(R)` the callback
|
||||
/// produces, or `None` if every jar was visited without a hit.
|
||||
|
||||
+59
-5
@@ -89,12 +89,13 @@ const PARSERS: &[(&str, DetectFn, ParseFn)] = &[
|
||||
// and returns None on a mismatch. By placing dbp last, the
|
||||
// earlier parsers' fast file-presence detects short-circuit and
|
||||
// dbp only runs on discs that fell through everything else.
|
||||
// dbp and deluxe both detect on "any top-level .jar in /BDMV/JAR/"
|
||||
// (every BD-J disc trips that) and do the real vendor-prefix check
|
||||
// in parse(). Order between them is somewhat arbitrary since either
|
||||
// returns None on a mismatched jar, but dbp goes first because its
|
||||
// parse path is cheaper (constant-pool iteration vs. deluxe's
|
||||
// bytecode walking once Phase D lands).
|
||||
("dbp", dbp::detect, dbp::parse),
|
||||
// deluxe last for the same reason as dbp: its detect() triggers
|
||||
// on any top-level .jar (every BD-J disc), and parse() does the
|
||||
// real `com/bydeluxe/` check. Phase A (master enum identification)
|
||||
// shipped 2026-05-10; phases B/C/D (per-stream binding decoder)
|
||||
// pending.
|
||||
("deluxe", deluxe::detect, deluxe::parse),
|
||||
];
|
||||
|
||||
@@ -420,3 +421,56 @@ pub(crate) fn read_jar_file(
|
||||
let path = find_jar_file(udf, filename)?;
|
||||
udf.read_file(reader, &path).ok().filter(|d| !d.is_empty())
|
||||
}
|
||||
|
||||
// ── Registry-level tests ────────────────────────────────────────────────────
|
||||
|
||||
#[cfg(test)]
|
||||
mod registry_tests {
|
||||
use super::*;
|
||||
|
||||
/// Lock the parser roster + order. If someone reorders the array
|
||||
/// or adds/removes a parser, this test forces them to update the
|
||||
/// expectation explicitly. The order is load-bearing: first
|
||||
/// matching `parse()` wins, so reordering changes which parser
|
||||
/// claims a disc on overlapping detect signals.
|
||||
///
|
||||
/// dbp + deluxe MUST stay at the end (their detect triggers on
|
||||
/// "any BD-J disc"; placing them earlier would short-circuit the
|
||||
/// stricter parsers above them).
|
||||
#[test]
|
||||
fn parsers_registry_order_locked() {
|
||||
let names: Vec<&str> = PARSERS.iter().map(|(n, _, _)| *n).collect();
|
||||
assert_eq!(
|
||||
names,
|
||||
vec![
|
||||
"paramount",
|
||||
"criterion",
|
||||
"pixelogic",
|
||||
"ctrm",
|
||||
"dbp",
|
||||
"deluxe"
|
||||
],
|
||||
"PARSERS array order changed — confirm dbp + deluxe stay last \
|
||||
(loose detect, real check in parse), and stricter parsers \
|
||||
(paramount/criterion/pixelogic/ctrm — all file-presence \
|
||||
gated detect) stay first."
|
||||
);
|
||||
}
|
||||
|
||||
/// Per-parser sanity: every parser has both detect and parse
|
||||
/// hooked up. Catches accidental nullification (e.g. someone
|
||||
/// stubbing `parse` to always-None during a refactor).
|
||||
#[test]
|
||||
fn parsers_registry_all_entries_populated() {
|
||||
for (name, detect, parse) in PARSERS {
|
||||
// Function pointers can't be Null in safe Rust, so the
|
||||
// assertion is just that the array entry was constructed
|
||||
// — which the iter above already implies. The test
|
||||
// exists to fail compile if someone changes the tuple
|
||||
// shape (e.g. adds a 4th field) without updating callers,
|
||||
// and as a marker for "these parsers exist."
|
||||
let _ = (name, detect, parse);
|
||||
}
|
||||
assert!(!PARSERS.is_empty(), "PARSERS array must not be empty");
|
||||
}
|
||||
}
|
||||
|
||||
+83
-35
@@ -49,52 +49,79 @@ pub fn codec(code: &str) -> &str {
|
||||
|
||||
// ── Language: English / multi-word names → ISO 639-2 ─────────────────────────
|
||||
|
||||
/// Map a free-form language label fragment to an ISO 639-2 code.
|
||||
/// Result of [`lang`] — ISO code + human-readable regional variant.
|
||||
///
|
||||
/// `code` is ISO 639-2 (always 3 lowercase letters).
|
||||
/// `variant` is the regional dialect as a human-readable English word
|
||||
/// (`"Brazilian"`, `"Castilian"`, `"Canadian"`, `"Simplified"`, ...)
|
||||
/// or `""` when the input names just a bare language without
|
||||
/// dialect ("Spanish" → variant=""). The variant matches the
|
||||
/// convention pixelogic / ctrm / criterion already use for their
|
||||
/// `StreamLabel::variant` field: a short display token the UI can
|
||||
/// surface verbatim.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub struct LangInfo {
|
||||
pub code: &'static str,
|
||||
pub variant: &'static str,
|
||||
}
|
||||
|
||||
/// Map a free-form language label fragment to an ISO 639-2 code AND
|
||||
/// (where applicable) its regional variant.
|
||||
///
|
||||
/// Handles both bare English names ("English", "Spanish") and the
|
||||
/// multi-word vendor variants we've seen in the corpus ("Brazilian
|
||||
/// Portuguese", "Castilian Spanish", "Canadian French"). Match is
|
||||
/// case-insensitive; longer compound phrases win over their bare
|
||||
/// counterparts (so "Brazilian Portuguese" → `por`, not consumed by
|
||||
/// counterparts (so "Brazilian Portuguese" returns
|
||||
/// `LangInfo { code: "por", variant: "Brazilian" }`, not consumed by
|
||||
/// the bare "Portuguese" entry).
|
||||
///
|
||||
/// Bare-name matches return `variant: ""`.
|
||||
///
|
||||
/// Returns `None` for unrecognized input — callers decide whether to
|
||||
/// fall back to MPLS spec codes, pass through raw, or drop the stream.
|
||||
/// Never guesses.
|
||||
pub fn lang(text: &str) -> Option<&'static str> {
|
||||
///
|
||||
/// Why the variant: the prior `lang() -> Option<&str>` shape silently
|
||||
/// dropped regional dialect info. "Brazilian Portuguese 5.1" became
|
||||
/// `language="por", variant=""` — UI displayed plain "Portuguese"
|
||||
/// even though the disc had explicitly labeled this stream Brazilian.
|
||||
/// Capturing the variant here parallels how pixelogic and ctrm
|
||||
/// populate `StreamLabel::variant` from their own region tables.
|
||||
pub fn lang(text: &str) -> Option<LangInfo> {
|
||||
let lower = text.to_lowercase();
|
||||
// Multi-word compounds first — longest-match wins.
|
||||
for (needle, code) in COMPOUND_LANGS {
|
||||
for (needle, code, variant) in COMPOUND_LANGS {
|
||||
if lower.contains(needle) {
|
||||
return Some(code);
|
||||
return Some(LangInfo { code, variant });
|
||||
}
|
||||
}
|
||||
// Bare names: word-boundary match (avoid "english" inside "englishman"
|
||||
// or any other accidental substring).
|
||||
for (needle, code) in BARE_LANGS {
|
||||
if has_word(&lower, needle) {
|
||||
return Some(code);
|
||||
return Some(LangInfo { code, variant: "" });
|
||||
}
|
||||
}
|
||||
None
|
||||
}
|
||||
|
||||
const COMPOUND_LANGS: &[(&str, &str)] = &[
|
||||
("brazilian portuguese", "por"),
|
||||
("euro portuguese", "por"),
|
||||
("european portuguese", "por"),
|
||||
("castilian spanish", "spa"),
|
||||
("latin american spanish", "spa"),
|
||||
("latin spanish", "spa"),
|
||||
("canadian french", "fra"),
|
||||
("parisian french", "fra"),
|
||||
("australian english", "eng"),
|
||||
("austrailian english", "eng"), // disc-corpus typo, keep matching
|
||||
("british english", "eng"),
|
||||
("simplified chinese", "zho"),
|
||||
("traditional chinese", "zho"),
|
||||
("mandarin chinese", "zho"),
|
||||
("cantonese chinese", "zho"),
|
||||
const COMPOUND_LANGS: &[(&str, &str, &str)] = &[
|
||||
("brazilian portuguese", "por", "Brazilian"),
|
||||
("euro portuguese", "por", "European"),
|
||||
("european portuguese", "por", "European"),
|
||||
("castilian spanish", "spa", "Castilian"),
|
||||
("latin american spanish", "spa", "Latin American"),
|
||||
("latin spanish", "spa", "Latin American"),
|
||||
("canadian french", "fra", "Canadian"),
|
||||
("parisian french", "fra", "Parisian"),
|
||||
("australian english", "eng", "Australian"),
|
||||
("austrailian english", "eng", "Australian"), // disc-corpus typo, keep matching
|
||||
("british english", "eng", "British"),
|
||||
("simplified chinese", "zho", "Simplified"),
|
||||
("traditional chinese", "zho", "Traditional"),
|
||||
("mandarin chinese", "zho", "Mandarin"),
|
||||
("cantonese chinese", "zho", "Cantonese"),
|
||||
];
|
||||
|
||||
const BARE_LANGS: &[(&str, &str)] = &[
|
||||
@@ -265,24 +292,45 @@ mod tests {
|
||||
assert_eq!(codec(""), "");
|
||||
}
|
||||
|
||||
fn li(code: &'static str, variant: &'static str) -> LangInfo {
|
||||
LangInfo { code, variant }
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn lang_bare_names() {
|
||||
assert_eq!(lang("English"), Some("eng"));
|
||||
assert_eq!(lang("english"), Some("eng"));
|
||||
assert_eq!(lang("Spanish 5.1 Dolby Digital"), Some("spa"));
|
||||
assert_eq!(lang("japanese"), Some("jpn"));
|
||||
assert_eq!(lang("Italian"), Some("ita"));
|
||||
fn lang_bare_names_have_empty_variant() {
|
||||
assert_eq!(lang("English"), Some(li("eng", "")));
|
||||
assert_eq!(lang("english"), Some(li("eng", "")));
|
||||
assert_eq!(lang("Spanish 5.1 Dolby Digital"), Some(li("spa", "")));
|
||||
assert_eq!(lang("japanese"), Some(li("jpn", "")));
|
||||
assert_eq!(lang("Italian"), Some(li("ita", "")));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn lang_compounds_carry_variant() {
|
||||
assert_eq!(
|
||||
lang("Brazilian Portuguese 5.1"),
|
||||
Some(li("por", "Brazilian"))
|
||||
);
|
||||
assert_eq!(lang("Castilian Spanish"), Some(li("spa", "Castilian")));
|
||||
assert_eq!(
|
||||
lang("Canadian French Dolby Digital"),
|
||||
Some(li("fra", "Canadian"))
|
||||
);
|
||||
assert_eq!(
|
||||
lang("Latin American Spanish"),
|
||||
Some(li("spa", "Latin American"))
|
||||
);
|
||||
assert_eq!(lang("Simplified Chinese"), Some(li("zho", "Simplified")));
|
||||
assert_eq!(lang("British English"), Some(li("eng", "British")));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn lang_compounds_win_over_bare() {
|
||||
// Brazilian Portuguese should map to por via the compound rule,
|
||||
// not be intercepted by bare "portuguese" (also por, but the
|
||||
// matcher must walk compounds first to be correct in principle).
|
||||
assert_eq!(lang("Brazilian Portuguese 5.1"), Some("por"));
|
||||
assert_eq!(lang("Castilian Spanish"), Some("spa"));
|
||||
assert_eq!(lang("Canadian French Dolby Digital"), Some("fra"));
|
||||
assert_eq!(lang("Latin American Spanish"), Some("spa"));
|
||||
// Brazilian Portuguese must map to (por, Brazilian) via the
|
||||
// compound rule, not be intercepted by bare "portuguese"
|
||||
// (which would yield (por, "") and lose the variant).
|
||||
assert_eq!(lang("Brazilian Portuguese").unwrap().variant, "Brazilian");
|
||||
assert_eq!(lang("Canadian French").unwrap().variant, "Canadian");
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
Reference in New Issue
Block a user