diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c3c400..1aeafa4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,34 @@ ### Fixed +- **A vendor label's forced-subtitle field was read as a boolean when it is an + enumeration, mislabelling full dialogue tracks and discarding the real forced + tracks.** One vendor's `playlists.xml` carries a per-subtitle-slot cell that + looks like a flag; the parser treated the value `1` as "this track is forced" + and every other value as "not forced". Measured across every image in the + corpus that uses this format — seven distinct discs — the cell takes four + values, and `1` is not the forced one: it marks a FULL dialogue track that + additionally contains some forced-narrative signs. Decoding three of those + discs and counting every PGS display set: all nine tracks bearing `1` on one + disc are full tracks of 949-1411 display sets, all seven on another are full + tracks of 1602-1651, and neither disc's `1` tracks are anything but full — + which is how a language ended up presenting as two identical full subtitle + tracks with one of them flagged forced. The values that DO name a dedicated + forced-narrative track, `2` and `3`, were being thrown away: they take their + own trailing stream slots, one per localized language, and measure 7 to 59 + display sets against the 1216-2655 of the full tracks they duplicate. So the + reading was wrong in both directions. The cell is now classified as the enumeration it is; only a + dedicated forced slot earns the flag, an unrecognised value never does, and + the "contains forced signs" value is dropped rather than weakened into a + forced label. This is not something content could have corrected afterwards: + clearing a wrong forced label requires a disc whose authoring sets + `forced_on_flag`, and measured discs in this format do not set it — so on + those discs the vendor cell was, and remains, the only evidence there is. + Four of the crate's own tests had been asserting the boolean reading. The + other two parsers that emit a forced qualifier from vendor metadata were + audited and are structurally immune — in both, the forced marker names a slot + of its own rather than hanging off a full track's entry — and are now pinned + by tests saying so. - **Content-based forced-subtitle detection never observed anything on a feature-length disc.** The PGS probe spent its entire 256 MiB budget on the first sectors of a title, where a feature has no subtitles at all — it hit diff --git a/src/labels/ctrm.rs b/src/labels/ctrm.rs index 6465eb8..86e2698 100644 --- a/src/labels/ctrm.rs +++ b/src/labels/ctrm.rs @@ -514,6 +514,47 @@ mod tests { assert_eq!(labels[0].qualifier, LabelQualifier::Forced); } + /// Immunity pin against the defect measured in the `paramount` parser, + /// where a vendor `forced_sub` cell hung off a FULL dialogue track's own + /// slot to say "this track also contains forced signs", and reading that + /// cell as "this track is forced" flagged 30 MB dialogue tracks forced. + /// + /// This format cannot express that. The forced signal is not a flag beside + /// a track's entry — it IS the entry's stream-kind token, drawn from a + /// closed vocabulary in which `subtitle_production` (the full dialogue + /// track) and `subtitle_narrative` (the forced-narrative track) are + /// mutually exclusive alternatives in the same position. A row is one or + /// the other; there is no cell a full track can carry to acquire the + /// qualifier, so the paramount failure mode has no encoding here. + /// + /// Mutation: give `subtitle_production` a `Forced` qualifier, or add a + /// forced side-flag that both kinds may carry. + #[test] + fn a_full_subtitle_track_kind_can_never_carry_the_forced_qualifier() { + // Every subtitle kind in the vocabulary, one row each. + let text = "id1,subtitle_production,1,eng\n\ + id2,subtitle_commentary,2,eng\n\ + id3,subtitle_dual,3,eng\n\ + id4,subtitle_bonus,4,eng\n\ + id5,subtitle_ime,5,kor\n\ + id6,subtitle_narrative,6,eng\n\ + id7,subtitle_ime_narrative,7,kor\n"; + let labels = parse_language_streams_text(text); + let forced: Vec<&str> = labels + .iter() + .filter(|l| l.qualifier == LabelQualifier::Forced) + .map(|l| l.language.as_str()) + .collect(); + assert_eq!( + forced.len(), + 2, + "only the two narrative kinds are forced, got {forced:?}" + ); + // The full dialogue kind specifically. + let production = parse_language_streams_text("id,subtitle_production,1,eng\n"); + assert_eq!(production[0].qualifier, LabelQualifier::None); + } + /// Spec: `subtitle_commentary` → Subtitle / Commentary. /// Mutation: treat as Normal → subtitle commentary not flagged. #[test] diff --git a/src/labels/paramount.rs b/src/labels/paramount.rs index 1debe7b..9dbc6b7 100644 --- a/src/labels/paramount.rs +++ b/src/labels/paramount.rs @@ -7,10 +7,12 @@ //! //! ``` +//! +//! `forced_sub` is an ENUMERATION, not a boolean — see [`ForcedSub`]. use super::{LabelPurpose, LabelQualifier, ParseResult, StreamLabel, StreamLabelType, xml}; use crate::sector::SectorSource; @@ -38,6 +40,64 @@ pub fn parse(reader: &mut dyn SectorSource, udf: &UdfFs) -> Option Some(ParseResult::high(labels)) } +/// One cell of the `forced_sub` CSV. +/// +/// The attribute reads like a boolean and was parsed as one (`cell == "1"` → +/// forced). It is not. Every image in the corpus carrying this vendor's +/// `playlists.xml` — seven distinct discs — uses four values, and decoding +/// three of those discs' feature subtitle tracks and counting every PGS +/// display set separates them into two populations two orders of magnitude +/// apart: +/// +/// * `0` — a subtitle track with no forced-narrative content. On the two +/// discs measured that use the flag at all, not one `0` track carried a +/// single `forced_on_flag` display set. +/// * `1` — a FULL DIALOGUE track that additionally contains some +/// forced-narrative signs. On one measured disc, all nine `1` cells are +/// full tracks of 949-1411 display sets, eight of them carrying 5-14 +/// flagged sets and the ninth none; that disc has no dedicated forced +/// track at all. On another, all seven `1` cells are full tracks of +/// 1602-1651 display sets carrying 0-31 flagged sets. Reading `1` as +/// forced is what made one language present as two identical full +/// subtitle tracks with one of them flagged forced. +/// * `2` and `3` — a DEDICATED forced-narrative track. These take their own +/// trailing STN slots, one per localized language, duplicating a language +/// that already holds a full track earlier in the list. Measured: the two +/// `2` slots on one disc are 15 and 10 display sets, EVERY one flagged +/// forced, against ~1600 on that disc's full tracks; the four `3` slots on +/// another are 7, 14, 23 and 59 display sets against 1216-2655. What +/// distinguishes `2` from `3` the corpus does not reveal — both sit in the +/// same trailing position, both measure the same shape, and one disc uses +/// each for a different language — so both map alike. +/// +/// So the old reading was wrong in BOTH directions: it flagged full dialogue +/// tracks forced, and it discarded the cells that name the real forced tracks. +/// +/// The `1` case is deliberately NOT carried through as a weaker "contains +/// forced segments" hint. There is no qualifier for that, and the asymmetry +/// argues against inventing one here: a wrong forced flag on a 30 MB dialogue +/// track is the user-visible defect, while a missing hint costs nothing. +/// +/// An unrecognised cell maps to [`ForcedSub::None`] — the conservative +/// direction, since asserting forced is the expensive mistake. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +enum ForcedSub { + /// No forced-narrative content, or an unrecognised cell. + None, + /// A full dialogue track that also carries forced-narrative segments. + ContainsForcedSegments, + /// A dedicated forced-narrative track. + ForcedNarrative, +} + +fn forced_sub_cell(cell: &str) -> ForcedSub { + match cell.trim() { + "1" => ForcedSub::ContainsForcedSegments, + "2" | "3" => ForcedSub::ForcedNarrative, + _ => ForcedSub::None, + } +} + /// Build the stream labels from a single `` feature /// element. Split out from `parse` so the per-type numbering and /// commentary/forced-index logic is unit-testable without a @@ -103,8 +163,8 @@ fn labels_from_feature(feature: &str) -> Vec { // Parse subtitle streams if let Some(sub) = xml::attr(feature, "sub") { - let forced: Vec = xml::attr(feature, "forced_sub") - .map(|s| s.split(',').map(|f| f.trim() == "1").collect()) + let forced: Vec = xml::attr(feature, "forced_sub") + .map(|s| s.split(',').map(forced_sub_cell).collect()) .unwrap_or_default(); // HashSet for the same reason as the audio side above: unbounded @@ -133,10 +193,12 @@ fn labels_from_feature(feature: &str) -> Vec { LabelPurpose::Normal }; - let qualifier = if forced.get(i).copied().unwrap_or(false) { - LabelQualifier::Forced - } else { - LabelQualifier::None + // Only a DEDICATED forced-narrative slot earns the forced flag. + // A cell marking a full track as merely containing forced segments + // is dropped, not weakened into a forced label (see [`ForcedSub`]). + let qualifier = match forced.get(i).copied().unwrap_or(ForcedSub::None) { + ForcedSub::ForcedNarrative => LabelQualifier::Forced, + ForcedSub::ContainsForcedSegments | ForcedSub::None => LabelQualifier::None, }; labels.push(StreamLabel { @@ -364,7 +426,7 @@ mod tests { // Subtitles: same shape, and the consequence is a misplaced forced // flag. `forced_sub` index 2 is the forced-narrative track; with the // empty slot renumbered away it would be written onto STN slot 2. - let feature = r#""#; + let feature = r#""#; let labels = labels_from_feature(feature); let s = subs(&labels); assert_eq!(s.len(), 2); @@ -411,10 +473,10 @@ mod tests { #[test] fn forced_sub_aligns_with_raw_csv_index() { - // sub="eng,eng,zho,ces" forced_sub="0,0,0,1": the forced flag is + // sub="eng,eng,zho,ces" forced_sub="0,0,0,3": the forced marker is // positional on the raw CSV, so 'ces' (index 3) is forced; its // stream_number is its 1-based cell position, 4. - let feature = r#""#; + let feature = r#""#; let labels = labels_from_feature(feature); let s = subs(&labels); assert_eq!(s.len(), 4); @@ -482,9 +544,9 @@ mod tests { /// Mutation: use stream_number (dense) instead of raw index → wrong subtitle forced. #[test] fn forced_sub_uses_raw_csv_index_with_gaps() { - // sub="eng,,fra,,spa" forced_sub="0,0,0,0,1" + // sub="eng,,fra,,spa" forced_sub="0,0,0,0,3" // raw CSV index 4 = "spa", i.e. STN slot 5. - let feature = r#""#; + let feature = r#""#; let labels = labels_from_feature(feature); let s = subs(&labels); assert_eq!(s.len(), 3); @@ -577,15 +639,92 @@ mod tests { assert_eq!(last, 300); } - /// Spec: forced_sub with whitespace around "1" must still parse as true. - /// Mutation: use `== "1"` instead of `trim() == "1"` → " 1 " fails. + /// Spec: a `forced_sub` cell with surrounding whitespace still classifies. + /// Mutation: drop the `trim()` → " 3 " falls through to the unrecognised + /// arm and the disc's forced-narrative track loses its label. #[test] - fn forced_sub_whitespace_around_one() { - let feature = r#""#; + fn forced_sub_cells_are_trimmed_before_classification() { + let feature = r#""#; let labels = labels_from_feature(feature); let s = subs(&labels); assert_eq!(s[0].qualifier, LabelQualifier::None); assert_eq!(s[1].qualifier, LabelQualifier::Forced); + assert_eq!(s[2].qualifier, LabelQualifier::None); + } + + /// `forced_sub` is an enumeration, and `1` is its "full dialogue track that + /// also carries forced signs" value — NOT "this track is forced". + /// + /// Measured on a disc whose feature declares nine `1` cells among 32 + /// subtitle slots: all nine are full dialogue tracks of 949-1411 display + /// sets, and the disc has no dedicated forced track at all. Reading `1` as + /// forced is what produced two identical full subtitle tracks for one + /// language with one of them flagged forced. + /// + /// Nothing downstream can undo this on the discs that need it most: + /// `mux::codec::pgs::demotable` may only clear a vendor forced label where + /// some track on the disc demonstrably sets `forced_on_flag`, and measured + /// discs using this label format never set it. + /// + /// Mutation: `"1" => ForcedNarrative` (the old reading) → red. + #[test] + fn a_contains_forced_segments_cell_is_not_a_forced_track() { + let feature = r#""#; + let labels = labels_from_feature(feature); + let s = subs(&labels); + assert_eq!(s.len(), 3); + assert!( + s.iter().all(|l| l.qualifier == LabelQualifier::None), + "a `1` marks a full track containing forced signs, not a forced track" + ); + } + + /// `2` and `3` are the cells that DO name a dedicated forced-narrative + /// track, and the old boolean reading discarded both. + /// + /// Measured: these cells occupy their own trailing STN slots, one per + /// localized language, duplicating a language that already holds a full + /// track earlier in the list. On one measured disc the four `3` slots carry + /// 7, 14, 23 and 59 display sets against 1216-2655 on the full tracks they + /// duplicate — and not one display set anywhere on that disc carries + /// `forced_on_flag`, so neither the scan probe nor the muxer can promote + /// them from content. The vendor cell is the only evidence there is. + /// + /// Mutation: drop either arm of the `"2" | "3"` match → red. + #[test] + fn a_dedicated_forced_narrative_cell_is_a_forced_track() { + // The measured shape: full tracks first, their forced companions in + // trailing slots of the same languages. + let feature = + r#""#; + let labels = labels_from_feature(feature); + let s = subs(&labels); + assert_eq!(s.len(), 5); + assert_eq!(s[1].qualifier, LabelQualifier::None, "the full cat track"); + assert_eq!(s[2].qualifier, LabelQualifier::None, "the full jpn track"); + assert_eq!(s[3].qualifier, LabelQualifier::Forced, "cat forced slot"); + assert_eq!(s[3].stream_number, 4); + assert_eq!(s[4].qualifier, LabelQualifier::Forced, "jpn forced slot"); + assert_eq!(s[4].stream_number, 5); + } + + /// An unrecognised cell must fall to NOT forced. Asserting forced is the + /// expensive mistake (a full dialogue track a player then burns on screen), + /// so an unknown value from a future authoring revision must not be able to + /// make that claim. + /// + /// Mutation: `_ => ForcedNarrative`, or treating "any non-zero" as forced. + #[test] + fn an_unrecognised_forced_sub_cell_is_not_forced() { + let feature = r#""#; + let labels = labels_from_feature(feature); + let s = subs(&labels); + assert_eq!(s.len(), 4); + assert!(s.iter().all(|l| l.qualifier == LabelQualifier::None)); + // ...and so must a cell the CSV simply does not reach. + let feature = r#""#; + let labels = labels_from_feature(feature); + assert_eq!(subs(&labels)[1].qualifier, LabelQualifier::None); } /// Spec: `find_feature_playlist` returns None when XML has no `` elements. diff --git a/src/labels/pixelogic.rs b/src/labels/pixelogic.rs index 30124e2..1c2951f 100644 --- a/src/labels/pixelogic.rs +++ b/src/labels/pixelogic.rs @@ -565,6 +565,60 @@ mod tests { assert_eq!(l.qualifier, LabelQualifier::Forced); } + /// Immunity pin against the defect measured in the `paramount` parser: a + /// vendor "forced" marker that sits on a FULL dialogue track's own slot to + /// mean "this track also contains forced signs", read as "this track is + /// forced" and so flagging full dialogue tracks forced. + /// + /// This grammar cannot express that. The forced marker is a component of a + /// slot's OWN token, so a forced-narrative pass occupies a slot of its own + /// (`{lang}_TXT_FOR_`, `{lang}_DUB_`) alongside the language's separate + /// full-dialogue slot — it is never a parallel array indexed against the + /// full tracks' slots, which is the shape that let one vendor's marker land + /// on a dialogue track. + /// + /// Mutation: give any full-dialogue component (`SDLG`, `TXT`, `SDH`, + /// `STRI`, `SCOM`) a forced qualifier of its own. + #[test] + fn a_full_subtitle_token_is_never_forced_without_its_own_forced_component() { + for token in [ + "eng_SDLG_", + "eng_TXT_", + "eng_SDH_", + "eng_STRI_", + "eng_SCOM_", + ] { + let l = parse_token_inner(token, None) + .unwrap_or_else(|| panic!("{token} must classify as a subtitle")); + assert_eq!(l.stream_type, StreamLabelType::Subtitle); + assert_ne!( + l.qualifier, + LabelQualifier::Forced, + "{token} carries no forced component and must not be forced" + ); + } + // And a language's forced pass is a SEPARATE slot from its full track, + // never a marker applied to the full track's slot. + let mut flag = UnknownParts::default(); + let tokens = strs(&[ + "FPL_MainFeature", + "PG Stream 1", + "eng_SDLG_", // PG slot 2 — the full dialogue track + "eng_TXT_FOR_", // PG slot 3 — its forced-narrative companion + ]); + let labels = assign_labels(&tokens, &mut flag); + let subs: Vec<_> = labels + .iter() + .filter(|l| l.stream_type == StreamLabelType::Subtitle) + .map(|l| (l.stream_number, l.qualifier)) + .collect(); + assert_eq!( + subs, + vec![(2, LabelQualifier::None), (3, LabelQualifier::Forced),], + "the forced marker belongs to its own slot, not to the full track's" + ); + } + #[test] fn parse_token_components_are_case_insensitive() { // Regression for the case-sensitive gate: a lowercase codec/