From 92f34a289ee3b80e5d0150c6a551e2b2fe37db33 Mon Sep 17 00:00:00 2001 From: MattJackson <1085847+MattJackson@users.noreply.github.com> Date: Sun, 10 May 2026 15:33:22 -0700 Subject: [PATCH] labels: refactor pixelogic + ctrm onto shared platform + hardening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the platform unification: every label parser now routes purpose/qualifier/codec classification through one source of truth (vocab.rs) instead of N hand-rolls, and every binary-blob byte scanner goes through one helper (text::extract_ascii_strings). pixelogic.rs: - Drop local extract_strings (~20 lines) — use text::extract_ascii_strings. - HARDENING: replace with skip-unknown-component + trace log. Pre-refactor behavior: any single uncatalogued token part (e.g. a future codec ID, new framework variant) silently dropped the entire stream record. New behavior: skip just the unknown part, surface what we know about the stream. - 8 new unit tests cover basic audio/subtitle paths, commentary, descriptive, region variant, the new skip-unknown-component regression, and the non-audio/non-subtitle early-out. ctrm.rs: - Replace with vocab::purpose(&name). Now word-boundary matched — 'Commenter Pro Track' no longer false-matches Commentary. - Replace with vocab::qualifier(&name). Same word-boundary tightening, plus picks up Forced and DescriptiveService for free. - Preserved structural commentary signal via as a fallback when name is silent (e.g. 'audio_commentary_1.name=Track 2'). - 6 new unit tests including the 'Commenter' false-positive regression and the SDH-only-on-subtitles boundary. text.rs: - Drop module-level dead_code allow now that pixelogic uses extract_ascii_strings. Net: all 5 framework parsers now on the unified platform. Future work (deluxe Phase D, paramount/criterion XML hardening) builds on the same scaffolding. Precommit green. --- src/labels/ctrm.rs | 176 ++++++++++++++++++++++++++++++++++++++-- src/labels/pixelogic.rs | 95 ++++++++++++++++++---- src/labels/text.rs | 5 -- 3 files changed, 246 insertions(+), 30 deletions(-) diff --git a/src/labels/ctrm.rs b/src/labels/ctrm.rs index 15ccee9..fdb9271 100644 --- a/src/labels/ctrm.rs +++ b/src/labels/ctrm.rs @@ -172,6 +172,164 @@ fn parse_language_streams(reader: &mut dyn SectorReader, udf: &UdfFs) -> Option< Some(labels) } +#[cfg(test)] +mod tests { + use super::*; + + /// Build a minimal menu_base.prop text and run `parse_menu_base`'s + /// inner logic via a temporary closure. This isolates the prop + /// parsing without needing a SectorReader. + fn parse_props(text: &str) -> Vec { + // Mirror the inner loop of parse_menu_base exactly. Kept + // separate so the test doesn't need disc fixtures. + use std::collections::HashMap; + let mut entries: HashMap> = HashMap::new(); + for line in text.lines() { + let line = line.trim(); + if line.is_empty() || line.starts_with('#') { + continue; + } + let Some(eq_pos) = line.find('=') else { + continue; + }; + let full_key = &line[..eq_pos]; + let value = &line[eq_pos + 1..]; + if let Some(dot_pos) = full_key.rfind('.') { + entries + .entry(full_key[..dot_pos].to_string()) + .or_default() + .insert(full_key[dot_pos + 1..].to_string(), value.to_string()); + } + } + let mut labels = Vec::new(); + for (prefix, props) in &entries { + let is_audio = props + .get("class") + .is_some_and(|c| c.contains("AudioButton")) + || prefix.starts_with("audio_"); + let is_subtitle = props + .get("class") + .is_some_and(|c| c.contains("SubtitleButton")) + || prefix.starts_with("subtitle_"); + let stream_num_str = props + .get("streamNumber") + .or_else(|| props.get("audioStream")) + .or_else(|| props.get("subtitleStream")); + let stream_num: u16 = match stream_num_str.and_then(|s| s.parse().ok()) { + Some(n) if n > 0 => n, + _ => continue, + }; + if !is_audio && !is_subtitle { + continue; + } + let name = props.get("name").cloned().unwrap_or_default(); + let purpose = match vocab::purpose(&name) { + LabelPurpose::Normal if prefix.contains("comm") => LabelPurpose::Commentary, + p => p, + }; + let qualifier = if is_subtitle { + vocab::qualifier(&name) + } else { + LabelQualifier::None + }; + let stream_type = if is_audio { + StreamLabelType::Audio + } else { + StreamLabelType::Subtitle + }; + let language = props + .get("audioLanguage") + .or_else(|| props.get("subtitleLanguage")) + .cloned() + .unwrap_or_default(); + labels.push(StreamLabel { + stream_number: stream_num, + stream_type, + language, + name, + purpose, + qualifier, + codec_hint: String::new(), + variant: String::new(), + }); + } + labels.sort_by_key(|l| (l.stream_type as u8, l.stream_number)); + labels + } + + #[test] + fn commentary_via_name() { + let labels = parse_props( + "audio_1.class=AudioButton\n\ + audio_1.streamNumber=2\n\ + audio_1.name=Director's Commentary\n\ + audio_1.audioLanguage=eng\n", + ); + assert_eq!(labels.len(), 1); + assert_eq!(labels[0].purpose, LabelPurpose::Commentary); + assert_eq!(labels[0].language, "eng"); + } + + #[test] + fn commentary_via_prefix_when_name_silent() { + let labels = parse_props( + "audio_commentary_1.class=AudioButton\n\ + audio_commentary_1.streamNumber=2\n\ + audio_commentary_1.name=Track 2\n\ + audio_commentary_1.audioLanguage=eng\n", + ); + assert_eq!(labels[0].purpose, LabelPurpose::Commentary); + } + + #[test] + fn commenter_does_not_false_match_commentary() { + // Regression for the pre-refactor `name.contains("comment")` + // bug: this would wrongly classify a "Commenter Pro" track as + // Commentary. vocab::purpose enforces a word boundary. + let labels = parse_props( + "audio_1.class=AudioButton\n\ + audio_1.streamNumber=2\n\ + audio_1.name=Commenter Pro Track\n\ + audio_1.audioLanguage=eng\n", + ); + assert_eq!(labels[0].purpose, LabelPurpose::Normal); + } + + #[test] + fn descriptive_via_name() { + let labels = parse_props( + "audio_1.class=AudioButton\n\ + audio_1.streamNumber=3\n\ + audio_1.name=English Descriptive Audio\n", + ); + assert_eq!(labels[0].purpose, LabelPurpose::Descriptive); + } + + #[test] + fn sdh_only_on_subtitles() { + // SDH applied to a subtitle stream. + let labels = parse_props( + "subtitle_1.class=SubtitleButton\n\ + subtitle_1.streamNumber=4\n\ + subtitle_1.name=English SDH\n", + ); + assert_eq!(labels[0].qualifier, LabelQualifier::Sdh); + } + + #[test] + fn sdh_not_applied_to_audio_stream_even_if_name_contains_sdh() { + // Audio streams should not pick up SDH (it's a subtitle + // concept). Edge case: badly-authored name happens to include + // "SDH" — we don't propagate it to audio metadata. + let labels = parse_props( + "audio_1.class=AudioButton\n\ + audio_1.streamNumber=5\n\ + audio_1.name=English SDH (track?)\n", + ); + assert_eq!(labels[0].qualifier, LabelQualifier::None); + } +} + // ── menu_base.prop parser ────────────────────────────────────────────────── fn parse_menu_base(reader: &mut dyn SectorReader, udf: &UdfFs) -> Option> { @@ -231,16 +389,20 @@ fn parse_menu_base(reader: &mut dyn SectorReader, udf: &UdfFs) -> Option LabelPurpose::Commentary, + p => p, }; - let qualifier = if is_subtitle && name_lower.contains("sdh") { - LabelQualifier::Sdh + // Qualifier: only apply to subtitles (SDH is a subtitle concept). + let qualifier = if is_subtitle { + vocab::qualifier(&name) } else { LabelQualifier::None }; diff --git a/src/labels/pixelogic.rs b/src/labels/pixelogic.rs index 9779624..a47584b 100644 --- a/src/labels/pixelogic.rs +++ b/src/labels/pixelogic.rs @@ -5,7 +5,7 @@ //! //! Token format: `{lang}_{codec?}_{purpose?}_{region?}_` -use super::{LabelPurpose, LabelQualifier, StreamLabel, StreamLabelType, vocab}; +use super::{LabelPurpose, LabelQualifier, StreamLabel, StreamLabelType, text, vocab}; use crate::sector::SectorReader; use crate::udf::UdfFs; @@ -22,7 +22,10 @@ pub fn detect(udf: &UdfFs) -> bool { pub fn parse(reader: &mut dyn SectorReader, udf: &UdfFs) -> Option> { let data = super::read_jar_file(reader, udf, "bluray_project.bin")?; - let strings = extract_strings(&data); + // min_len=4 matches the prior local extract_strings impl. The token + // grammar is `{lang3}_{codec?}_{purpose?}_{region?}_` so the + // shortest meaningful run is 4 chars (lang + underscore). + let strings = text::extract_ascii_strings(&data, 4); let mut labels = Vec::new(); let mut in_feature = false; @@ -127,7 +130,13 @@ fn parse_token(s: &str) -> Option { } else if part.starts_with("PGStream") { is_subtitle = true; } else { - return None; + // Unknown token component — skip this single part rather + // than discarding the entire stream record. Pre-refactor + // behavior was `return None` here, which silently dropped + // any stream containing a single uncatalogued token (e.g. + // a new codec ID or framework variant). Better to surface + // what we know than discard a whole stream over one part. + tracing::debug!(part = %part, "pixelogic: unrecognized token component, skipping"); } } @@ -153,22 +162,72 @@ fn parse_token(s: &str) -> Option { }) } -fn extract_strings(data: &[u8]) -> Vec { - let mut strings = Vec::new(); - let mut current = String::new(); +// extract_strings removed — replaced by super::text::extract_ascii_strings(data, 4). - for &b in data { - if (0x20..0x7f).contains(&b) { - current.push(b as char); - } else { - if current.len() > 3 { - strings.push(current.clone()); - } - current.clear(); - } +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parse_token_basic_audio() { + let l = parse_token("eng_MLP_").unwrap(); + assert_eq!(l.stream_type, StreamLabelType::Audio); + assert_eq!(l.language, "eng"); + assert_eq!(l.codec_hint, "TrueHD"); + assert_eq!(l.purpose, LabelPurpose::Normal); } - if current.len() > 3 { - strings.push(current); + + #[test] + fn parse_token_basic_subtitle_sdh() { + let l = parse_token("eng_SDH_").unwrap(); + assert_eq!(l.stream_type, StreamLabelType::Subtitle); + assert_eq!(l.language, "eng"); + assert_eq!(l.qualifier, LabelQualifier::Sdh); + } + + #[test] + fn parse_token_commentary() { + let l = parse_token("eng_MLP_ACOM_").unwrap(); + assert_eq!(l.stream_type, StreamLabelType::Audio); + assert_eq!(l.purpose, LabelPurpose::Commentary); + } + + #[test] + fn parse_token_descriptive() { + let l = parse_token("eng_AC3_ADES_").unwrap(); + assert_eq!(l.purpose, LabelPurpose::Descriptive); + } + + #[test] + fn parse_token_with_region() { + let l = parse_token("eng_MLP_US_").unwrap(); + assert_eq!(l.language, "eng"); + assert_eq!(l.variant, "US"); + } + + #[test] + fn parse_token_unknown_component_does_not_kill_stream() { + // Regression: pre-refactor, an unrecognized token part returned + // None for the whole stream, silently dropping it. New + // behavior: skip the unknown part, surface what we know. + let l = parse_token("eng_MLP_FUTUREFLAG_FOR_").unwrap(); + assert_eq!(l.stream_type, StreamLabelType::Audio); + assert_eq!(l.language, "eng"); + assert_eq!(l.codec_hint, "TrueHD"); + assert_eq!(l.qualifier, LabelQualifier::Forced); + } + + #[test] + fn parse_token_no_audio_or_subtitle_signal_returns_none() { + // A token that has only a language and an unknown part with + // no audio/subtitle classifier should still return None — + // there's no way to file it as a stream. + assert!(parse_token("eng_UNKNOWN_").is_none()); + } + + #[test] + fn parse_token_rejects_non_lang_prefix() { + assert!(parse_token("XX_MLP_").is_none()); + assert!(parse_token("ENG_MLP_").is_none()); // uppercase not accepted as ISO 639-2 } - strings } diff --git a/src/labels/text.rs b/src/labels/text.rs index 921c29d..40cd949 100644 --- a/src/labels/text.rs +++ b/src/labels/text.rs @@ -9,11 +9,6 @@ //! more structured parse path (e.g. `class_reader` for .class) should //! prefer that — this helper is for genuinely unstructured input. -// Staged for the pixelogic refactor: pixelogic still has its own -// extract_strings copy; this is the shared replacement waiting for -// the refactor. dead-code allow comes off when pixelogic switches. -#![allow(dead_code)] - /// Walk `data`, emit every maximal run of printable-ASCII bytes /// (`0x20..=0x7E`) whose length is at least `min_len`. ///