labels: refactor pixelogic + ctrm onto shared platform + hardening

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.
2026-05-10 15:33:22 -07:00
parent 1d46ad02b6
commit 5c3169b4e5
3 changed files with 246 additions and 30 deletions
@@ -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<StreamLabel> {
+        // 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<String, HashMap<String, String>> = 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<Vec<StreamLabel>> {
@@ -231,16 +389,20 @@ fn parse_menu_base(reader: &mut dyn SectorReader, udf: &UdfFs) -> Option<Vec<Str
        }

        let name = props.get("name").cloned().unwrap_or_default();
-        let name_lower = name.to_lowercase();

-        let purpose = if name_lower.contains("comment") || prefix.contains("comm") {
-            LabelPurpose::Commentary
-        } else {
-            LabelPurpose::Normal
+        // Purpose: ask vocab first (word-boundary matched — avoids the
+        // "Commenter" false positive the prior `name.contains("comment")`
+        // had). Then fall back to the structural prefix check
+        // (`audio_commentary.foo`-style keys group commentary streams
+        // regardless of display name).
+        let purpose = match vocab::purpose(&name) {
+            LabelPurpose::Normal if prefix.contains("comm") => 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
        };
@@ -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<Vec<StreamLabel>> {
    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<StreamLabel> {
        } 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<StreamLabel> {
    })
 }

-fn extract_strings(data: &[u8]) -> Vec<String> {
-    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
 }
@@ -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`.
 ///