[ptr727]

Summary

FFmpeg and HandBrake drop the LanguageIETF element of every track when they write an MKV file. A region or script subtag, such as the CA in fr-CA, then cannot come back from the ISO 639-2 code alone. Before this change only the ReEncode and DeInterlace steps restored the tags. Issue: #1148.

• Add Convert.RestoreLanguageTags(), the one restore every FFmpeg and HandBrake rewrite calls before its output replaces the original.
  • It reads the output's mkvmerge -J JSON and checks that each track keeps its number, type, and ISO 639-2 tag. Otherwise it logs an error and the rewrite fails before the move, so the original stays. A rewrite that drops or reorders tracks therefore never replaces the original, and never gets the tags on the wrong tracks.
  • It compares the raw JSON tracks rather than parsing them into MediaProps, which would log a "Setting IETF tag from ISO639 tag" warning for every track the rewrite stripped.
• Call it from the paths that did not restore: the lossless timestamp repair (after the regression gate and re-verify, before the move), closed caption removal, the re-encode repair tier (FFmpeg or HandBrake), and the Convert.ReMuxToMkv() FFmpeg fallback. The fallback restores only for a Matroska input, since another container carries no IETF tags.
• ConvertToMkv() and DeInterlace() call the same helper in place of their own SetTrackLanguage() call. Their VerifyTrackOrder() check after the move is removed, along with MediaProps.VerifyTrackOrder(). That check failed the step only after the original was already replaced, and hit Debug.Assert. The helper's check before the move replaces it, so one track comparison remains.
• Every HandBrake run goes through ProcessFile.ConvertUsingHandBrake(), the workaround for HandBrake converts all subtitles to ASS vs. passthrough #95 and HandBrake removes IETF language tags #149 that DeInterlace() already used. HandBrake encodes without subtitles, MkvMerge merges the original subtitle tracks back unchanged, and the tags are restored. The repair by re-encode's HandBrake fallback now uses it as well. It used to select every subtitle, so HandBrake converted them to ASS and added embedded closed captions as one more subtitle track. HandBrake.ConvertToMkv() no longer takes an includeSubtitles flag, and HandBrakeBuilder.AllSubtitles() is removed.
• HandBrake writes the video, then the audio, then the subtitle tracks. Convert.GetHandBrakeTrackOrder() numbers copies of the tags in that order for the re-encode repair tier's HandBrake fallback and for DeInterlace(). DeInterlace() used to renumber MkvMergeProps' own track objects in place through the shallow MediaProps.Clone(). That method has no caller left and is removed.
• ReMuxToMkv(string, out string) takes the input's MkvMerge props, or null where they are not read yet. RemuxByExtension() runs before GetMediaProps(), so it passes none. The track-selection overload no longer accepts null, and its one null caller now picks the overload itself.
• ARCHITECTURE.md states the restore under "Language processing" and draws it in the remux, closed caption, and verify and repair flowcharts.

The branch carries develop with #1149, the #1146 fix, merged in. The lossless repair then runs setts, the MkvMerge remux #1149 adds, the regression gate, the re-verify, this restore, and the move, in that order.

Verification

• dotnet test: 473 passed, on the branch merged with current develop. New tests cover:

  • the helper with matching tracks, and with a changed track number, type, or ISO language, or a dropped track
  • an identify that fails or returns unparsable JSON
  • closed caption removal, with the restore passing or failing before the move
  • the FFmpeg remux fallback, for Matroska and non-Matroska input
  • RemuxByExtension() on an .avi before the media props load, with the FFmpeg fallback taking over. It fails with a NullReferenceException if the null guard is reverted.
  • GetHandBrakeTrackOrder() on a video, subtitle, audio input, leaving the original numbers unchanged
  • the HandBrake repair fallback with and without a subtitle track: HandBrake runs with --subtitle none, MkvMerge merges the original back only when there is a subtitle, and the restore runs on .tmp12

  The re-encode repair test also asserts the restore ran on .tmp12. The fake MkvMerge answers --identify with the sample track list through one FakeTools.SampleIdentifyCase() helper.

• Real media, ptr727/plexcleaner:develop image tools (FFmpeg 8.1.2, MkvToolNix 100.0). The clip is synthetic: H.264 video tagged en and AAC audio tagged fr-CA, with one audio packet timestamp stepping back. The default settings were used with ReEncode off, so verify takes the lossless setts repair.

  Build	Repair	Audio language_ietf after process
  Image's own build (develop before this change)	lossless, then a metadata-repair remux	fr
  This branch	lossless	fr-CA

  The baseline also logs "Setting IETF tag from ISO639 tag" for each track and runs an extra "Remux to repair metadata errors" pass. This branch does neither. Rerun on the branch merged with Drop Tags in the Lossless Timestamp Repair Like Every Other Rewrite #1149: fr-CA still holds.

• HandBrake 1.11.2 from the same image, on a synthetic HEVC clip with CC608 caption SEI, AAC audio, and one SRT subtitle:

  • --all-subtitles writes SRT plus an extra SubStationAlpha track from the captions.
  • --subtitle none, followed by the MkvMerge merge of the original subtitles, gives exactly the original video, audio, and SRT tracks.

• CSharpier, dotnet format style --verify-no-changes --severity=info, markdownlint, and cspell are clean. The prose gate reports only comment-added, which the comments label covers.

Not covered by a unit test: the lossless repair call site. No test drives that tier to a successful repair today, and #1149 changes the fakes such a test needs. That test is #1151. The real-media run above exercises this call site.

Local Strict Review

The first pass raised these findings:

• Fixed: the rewrite sites disagreed on what a track mismatch means. Every site now fails before the move, per the maintainer's answer.
• Fixed: HasSameTracks() duplicated MediaProps.VerifyTrackOrder(). The latter is removed, together with its callers after the move.
• Fixed: FakeTools had two copies of the sample identify arm and the sample sidecar read.
• Fixed: closed caption removal had no restore test.
• Fixed: the mismatch test covered only the number arm.
• Deferred to Test the Lossless Timestamp Repair Through a Successful Repair #1151: no restore test for the lossless repair.
• Declined as a style preference: switching GetMkvMergeTrackType() from a type pattern to the TrackType enum. Both throw on an unknown track, and there is no existing type-to-string mapping to share. The MkvMergeTool switch maps strings to new props objects.

The second pass raised one finding, now fixed. Once a mismatch fails the rewrite, the re-encode tier's HandBrake fallback failed for any file not already in video, audio, subtitle order.

The third pass raised one finding, now fixed. The HandBrake fallback's --all-subtitles adds embedded closed captions as an extra subtitle track, which failed the track check. The fix routes every HandBrake run through ConvertUsingHandBrake(), per the maintainer's direction.

Later passes over the HandBrake helper raised a dropped merge log line and a merge test that did not check the track selection. Both are fixed. The passes over the null fix and the #1149 merge raised nothing.

Hosted Review

• Fixed in 1503874: embedded captions blocked the HandBrake repair, and the HandBrake fallback added untracked caption tracks.
• Fixed in da26844: RemuxByExtension() passed a null MkvMergeProps to the FFmpeg fallback.
• Deferred to Test the Lossless Timestamp Repair Through a Successful Repair #1151: a successful lossless repair test.

Dropped findings

• PlexCleanerTests/CleanupStepRepeatTests.cs: a third hand-written copy of the sample identify arm, on a file this change does not touch (consolidation, pre-existing).

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 6 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 7b49921f-b791-4942-a106-3e40f4d12f06

Commits

Reviewing files that changed from the base of the PR and between 4818ea2 and 8cebfc0.

Files selected for processing: 10

• ARCHITECTURE.md
• PlexCleaner/Convert.cs
• PlexCleaner/HandBrakeBuilder.cs
• PlexCleaner/HandBrakeTool.cs
• PlexCleaner/MediaProps.cs
• PlexCleaner/ProcessFile.cs
• PlexCleanerTests/FakeTools.cs
• PlexCleanerTests/ReEncodeBitrateStateTests.cs
• PlexCleanerTests/ReplaceOriginalTests.cs
• PlexCleanerTests/VerifyToolErrorTests.cs

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[ptr727]

A recorded local strict-review pass covers head 1ddcbb2b4808df393d23cadda99fa2e40d298cd8, the content this pull request carries at that commit against develop, and it recorded 1 finding.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Restore IETF language tags before media rewrites replace originals

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Restore IETF language tags after FFmpeg and HandBrake rewrites, including timestamp repair and
 remux fallback.
• Reject outputs with mismatched tracks before they replace the original file.
• Account for HandBrake track ordering and add coverage for restoration and failure paths.

Diagram

graph TD
  A["Original tags"] --> B["FFmpeg or HandBrake"] --> C["Temporary MKV"] --> D["MkvMerge identify"] --> E{"Tracks match?"} -->|yes| F["MkvPropEdit restore"] -->|success| G["Replace original"]
  A -->|expected tracks| E
  A -->|IETF tags| F
  D -->|identify fails| H["Reject rewrite"]
  E -->|no| H
  F -->|failure| H

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Map changed tracks instead of rejecting the rewrite

• ➕ Could accept outputs that reorder tracks or add a subtitle.
• ➖ Matching transcoded tracks reliably is difficult; ambiguity could put language tags on the wrong tracks.

Recommendation: Keep the pre-replacement, fail-closed validation: it protects both the original file and track-language correctness. Resolve the identified HandBrake embedded-caption case explicitly before accepting additional tracks, rather than weakening the general match check.

Files changed (8) +343 / -173

Bug fix (2) +113 / -78

Convert.csCentralize validated IETF tag restoration +89/-10

Centralize validated IETF tag restoration

• Adds raw-JSON track checks before MkvPropEdit restores tags and adds a non-mutating HandBrake track-order mapping. Uses the helper for conversion and the Matroska FFmpeg remux fallback, while making remux overload selection explicit.

PlexCleaner/Convert.cs

ProcessFile.csRestore tags before repair outputs replace originals +24/-68

Restore tags before repair outputs replace originals

• Adds restoration to closed-caption removal, lossless timestamp repair, and FFmpeg or HandBrake re-encode repair. Uses HandBrake-ordered tag copies where needed and removes checks that previously ran after replacement.

PlexCleaner/ProcessFile.cs

Refactor (1) +0 / -64

MediaProps.csRemove obsolete cloning and post-move track check +0/-64

Remove obsolete cloning and post-move track check

• Deletes the shallow Clone method and VerifyTrackOrder now that callers prepare independent HandBrake track properties and validate output before replacement.

PlexCleaner/MediaProps.cs

Tests (4) +219 / -19

FakeTools.csShare sample identify responses across fake tools +27/-16

Share sample identify responses across fake tools

• Adds a reusable MkvMerge identify script case and consolidates sample sidecar reading so rewrite tests can exercise track validation.

PlexCleanerTests/FakeTools.cs

ReEncodeBitrateStateTests.csAssert repair-tier tag restoration runs +3/-0

Assert repair-tier tag restoration runs

• Records MkvPropEdit arguments and verifies that the re-encode repair tier restores tags on its temporary output.

PlexCleanerTests/ReEncodeBitrateStateTests.cs

ReplaceOriginalTests.csCover track validation and pre-replacement restoration +183/-2

Cover track validation and pre-replacement restoration

• Tests matching and mismatched tracks, identify failures, and non-mutating HandBrake ordering. Also checks closed-caption replacement behavior and Matroska-only restoration in the FFmpeg remux fallback.

PlexCleanerTests/ReplaceOriginalTests.cs

VerifyToolErrorTests.csSupport restoration in repair error tests +6/-1

Support restoration in repair error tests

• Makes the fake MkvMerge answer identify requests and provides a successful fake MkvPropEdit so the repair scenario can pass the new restoration step.

PlexCleanerTests/VerifyToolErrorTests.cs

Documentation (1) +11 / -12

ARCHITECTURE.mdDocument tag restoration across rewrite flows +11/-12

Document tag restoration across rewrite flows

• Describes pre-replacement track validation, HandBrake ordering, and the Matroska-only remux fallback. Updates flowcharts to place restoration before replacement.

ARCHITECTURE.md

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (1) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Embedded captions block repair ✓ Resolved

Description

RepairAndReVerify() passes HandBrake’s output to RestoreLanguageTags() with a track list derived
from the original container, and the exact-count check rejects any additional track. When
AllSubtitles() extracts embedded EIA-608 captions as a new subtitle track, the otherwise repaired
temporary file is deleted before it can replace the original, and the file is marked RepairFailed.

Code

PlexCleaner/ProcessFile.cs[R2410-2413]

+        if (
+            VerifyMediaStreams(new FileInfo(tempName)) != VerifyResult.Clean
+            || !Convert.RestoreLanguageTags(tempName, languageProps)
+        )

Evidence

The HandBrake fallback requests all subtitles, which can materialize embedded captions as an
additional subtitle track; the reported HandBrake 1.11.2 case produces a SubStationAlpha track from
CC608 SEI. RestoreLanguageTags() compares the output track count with the list derived from the
original container, and a failed comparison causes the repair path to delete the temporary file and
return false.

Restore IETF language tags after FFmpeg and HandBrake rewrites
PlexCleaner/ProcessFile.cs[2397-2413]
PlexCleaner/Convert.cs[232-244]
ARCHITECTURE.md[807-813]
PlexCleaner/HandBrakeTool.cs[112-139]
PlexCleaner/Convert.cs[233-244]
PlexCleaner/ProcessFile.cs[2376-2419]
PlexCleaner/HandBrakeTool.cs[125-140]
PlexCleaner/ProcessFile.cs[2376-2417]
PlexCleaner/Convert.cs[204-244]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
HandBrake’s all-subtitles fallback can create a track from embedded captions that is absent from the original track list. The exact-count check then rejects an otherwise repaired output.
## Fix Focus Areas
- PlexCleaner/ProcessFile.cs[2376-2419]
- PlexCleaner/Convert.cs[176-244]
- PlexCleaner/HandBrakeTool.cs[112-139]
## Recommended Fix
Choose and implement a handling strategy for HandBrake-generated caption tracks: prevent their extraction by selecting explicit subtitle tracks, remux the output to retain only expected tracks, or identify and handle the additional tracks during restoration. Continue validating every original track without assigning its IETF language tag to the wrong output track, preserve the pre-move failure guarantee, and add coverage for inputs with embedded captions.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Remuxing non-MKV files crashes when FFmpeg takes over ✓ Resolved

Description

RemuxByExtension() now passes MkvMergeProps to Convert.ReMuxToMkv(), but nothing has loaded
that property yet: it is initialised to null!, and Process and MkvProcess.ReMux call
RemuxByExtension() before GetMediaProps(). When MkvMerge fails on an AVI, MP4 or other
extension-matched file and the FFmpeg fallback succeeds, languageProps.IsContainerMkv() throws a
NullReferenceException, so the very files this fallback exists for now fail instead of being
converted.

Code

PlexCleaner/ProcessFile.cs[218]

+        if (!Convert.ReMuxToMkv(FileInfo.FullName, MkvMergeProps, out string outputName))

Evidence

MkvMergeProps is declared = null! and is only assigned in Refresh(), which GetMediaProps()
calls. Process.cs calls RemuxByExtension at line 108 but GetMediaProps only at line 154, and
MkvProcess.ReMux never calls GetMediaProps. In Convert.ReMuxToMkv, the FFmpeg fallback branch
then dereferences languageProps.IsContainerMkv(). The new test passes an already populated
processFile.MkvMergeProps, so it does not cover this path.

PlexCleaner/ProcessFile.cs[45-45]
PlexCleaner/ProcessFile.cs[188-228]
PlexCleaner/Process.cs[105-159]
PlexCleaner/MkvProcess.cs[5-10]
PlexCleaner/Convert.cs[98-112]
PlexCleaner/ProcessFile.cs[2694-2720]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`RemuxByExtension()` runs before `GetMediaProps()`, so `MkvMergeProps` is still null when it calls `Convert.ReMuxToMkv(FileInfo.FullName, MkvMergeProps, ...)`. When FFmpeg takes over after MkvMerge fails, `languageProps.IsContainerMkv()` throws a NullReferenceException.
## Fix Focus Areas
- PlexCleaner/Convert.cs[49-115]
- PlexCleaner/ProcessFile.cs[188-228]
## Recommended Fix
Change the `ReMuxToMkv` parameter to `MediaProps? languageProps` and guard the restore: `if (languageProps is { } props && props.IsContainerMkv() && !RestoreLanguageTags(tempName, props))`. Pass `null` from `RemuxByExtension()`, since the props are not loaded there. An extension-matched input is not Matroska anyway. Add a test that calls `RemuxByExtension` on a fresh `ProcessFile` and forces the FFmpeg fallback.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Lossless repair lacks failure coverage 📎 Requirement gap ☼ Reliability

Description

TryLosslessTimestampRepair() now restores language tags on the temporary output before moving it
over the original, but the added tests do not drive this successful repair path or a failed tag
edit. Without that coverage, the required edit-before-move and original-preservation behavior for
this tier remains unverified.

Code

PlexCleaner/ProcessFile.cs[R2585-2589]

+        if (!Convert.RestoreLanguageTags(tempName, MkvMergeProps))
+        {
+            File.Delete(tempName);
+            return false;
+        }

Evidence

The implementation adds the required restore call to the lossless repair path, but the new test
additions cover direct restoration, closed-caption removal, the remux fallback, and re-encode
behavior; they do not exercise TryLosslessTimestampRepair() or the .tmp14 failure cleanup
required by the rule.

Test the successful lossless timestamp repair and failed tag edit
PlexCleaner/ProcessFile.cs[2585-2592]
PlexCleanerTests/ReplaceOriginalTests.cs[192-369]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The lossless timestamp repair now depends on `RestoreLanguageTags()` succeeding before the temporary file replaces the original, but no test exercises either the successful path or failed-edit cleanup.
## Fix Focus Areas
- PlexCleaner/ProcessFile.cs[2585-2589]
- PlexCleanerTests/ReplaceOriginalTests.cs[192-369]
## Recommended Fix
Add a fake-tool test that drives `TryLosslessTimestampRepair()` through a successful audio-only repair, verifies the regression gate and clean re-verification complete before `MkvPropEdit` edits `.tmp14`, and verifies that a nonzero `MkvPropEdit` exit leaves the original byte-identical with no temporary files.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[Copilot]

🟡 Changes recommended

An active moderate issue can cause otherwise repairable files to fail during the HandBrake fallback, and the lossless-repair path lacks regression coverage for its critical pre-replacement edit.

2 open issues

• Restrict HandBrake fallback to the original subtitle selection · New
• Add regression coverage for lossless repair before replacing the original · New

What changed in this PR

This pull request restores IETF language tags after FFmpeg and HandBrake rewrites, validating track identity before rewritten output replaces the original.

Changes:

• Adds Convert.RestoreLanguageTags() for pre-replacement track validation and language restoration.
• Applies restoration across remux, closed-caption removal, re-encode, deinterlace, and lossless timestamp-repair paths.
• Adds HandBrake track ordering, fake-tool coverage, regression tests, and architecture documentation.
• Identifies a HandBrake fallback issue with embedded caption tracks and a missing successful lossless-repair regression test.

This pull request requires changes.

File	Description
PlexCleanerTests/​VerifyToolErrorTests.cs	Updates repair fakes for identification and language-edit operations.
PlexCleanerTests/​ReplaceOriginalTests.cs	Tests restoration, HandBrake ordering, closed-caption handling, remux fallback, and replacement safety.
PlexCleanerTests/​ReEncodeBitrateStateTests.cs	Verifies language restoration edits the re-encode temporary output.
PlexCleanerTests/​FakeTools.cs	Consolidates sample identify data and fake-tool setup.
PlexCleaner/​ProcessFile.cs	Integrates restoration into rewrite and repair workflows and removes obsolete post-move validation.
PlexCleaner/​MediaProps.cs	Removes obsolete cloning and track-order verification helpers.
PlexCleaner/​Convert.cs	Adds language restoration, track validation, HandBrake ordering, and updated remux handling.
ARCHITECTURE.md	Documents language restoration and updates processing flowcharts.

_{Review effort: Lite}

[ptr727]

A recorded local strict-review pass covers head 8cebfc02faf60efe80ebd6ebb07debf59e1ce90c, the content this pull request carries at that commit against develop, and it recorded 0 findings.

[ptr727]

Answers to Qodo's findings on 1ddcbb2:

1. Embedded captions block repair: fixed in 1503874. Every HandBrake run goes through ConvertUsingHandBrake(), which encodes with --subtitle none, merges the original subtitle tracks back with MkvMerge, and restores the tags.
2. Remuxing non-MKV files crashes when FFmpeg takes over: fixed in da26844. ReMuxToMkv() takes optional language props, and RemuxByExtension() passes none, since it runs before GetMediaProps(). RemuxByExtension_FfMpegFallbackBeforeMediaProps_RemuxesWithoutRestore covers it, and fails with the NullReferenceException if the guard is reverted.
3. Lossless repair lacks failure coverage: deferred to Test the Lossless Timestamp Repair Through a Successful Repair #1151, a fake-tool test of a successful lossless repair and of a failed restore before the move. It waits on the Drop Tags in the Lossless Timestamp Repair Like Every Other Rewrite #1149 gate changes, which this branch now carries.