[ptr727]

Summary

HandBrake runs with --subtitle none, and MkvMerge merges back only the original's subtitle tracks. EIA-608/CTA-708 closed captions embedded in the video stream are not a track, so a HandBrake rewrite drops them and nothing reported it. Both HandBrake callers reach this: DeInterlace(), and the HandBrake fallback of the re-encode repair since #1152 routed it through ConvertUsingHandBrake(). Issue: #1157.

The drop is kept, the documented behavior since the HandBrake subtitle workaround, and made visible:

• ConvertUsingHandBrake() calls a new ReportClosedCaptionsDropped() before the HandBrake run. It scans for embedded closed captions with the existing FindClosedCaptionTracks(). It logs HandBrake drops the Closed Captions in the video stream when the video carries them, and Unknown if HandBrake drops Closed Captions when the scan fails. A failed scan never blocks the rewrite, and a cancelled one returns.
• The scan ignores the Verified and VerifyFailed skips of a conditional scan, since those states do not imply a scan ran with RemoveClosedCaptions off, which is exactly when the captions are still there. It is skipped when ScannedCaptions or ClearedCaptions records a real scan or a removal, so a process run that just scanned the file does not decode it again.
• FindClosedCaptionTracks() takes a recordScan parameter, default true. This caller passes false, so a clean scan does not write ScannedCaptions to the sidecar in the middle of a rewrite.
• The scan runs in its own ToolErrorScope. A killed, cancelled, or unlaunchable scan therefore does not count as a repair tool error, which would otherwise leave a failed repair without RepairFailed and repeat it every run.
• HISTORY.md 3.23 and Docs/ClosedCaptions.md "HandBrake Rewrites" state the drop and the warning.

With RemoveClosedCaptions on, the default, process removes the captions before either step runs. So the exposure is that option off, or the deinterlace and reencode commands run directly.

Verification

• dotnet test: 491 passed. DeInterlace_ClosedCaptions_WarnsWithoutRecordingScan covers captions present (the warning), absent (no warning), and a failed or signal-killed caption probe (the unknown warning). Each case asserts ScannedCaptions stays clear and the outer tool-error count stays 0. DeInterlace_CaptionsAlreadyScanned_SkipsScan covers the skip for both states.

• Negative control: without the scan's own scope, the signal-killed case and 6 existing VerifyToolErrorTests fail, the tests whose fakes leave ffprobe out.

• Real media in ptr727/plexcleaner:latest (FFmpeg 8.1.2, HandBrake 1.11.2), this branch published framework-dependent per OPERATIONS.md. The inputs are synthetic: synthesize.py hdr10-cc (HEVC with CC608 SEI) and a testsrc2 H.264 clip, each given a Matroska field-order flag so deinterlace takes the HandBrake path.

  Input	closed_captions before	Warning	closed_captions after	Sidecar state
  HEVC with CC608 SEI	1	HandBrake drops the Closed Captions in the video stream : Format: "hevc"	0	ReMuxed, DeInterlaced
  H.264, no captions	0	none	0	ReMuxed, DeInterlaced

Local strict review: two frontier passes. The first raised three findings, all introduced and all fixed in 2b33763: the repair tool-error accounting, the redundant scan on a known state, and an ambiguous doc sentence, plus two sentence-length lint hits. The second pass over the full diff found nothing.

🤖 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 17 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: 730c2026-d862-4708-b425-763e6e4faea5

Commits

Reviewing files that changed from the base of the PR and between 423afab and 57d7858.

Files selected for processing: 4

• Docs/ClosedCaptions.md
• HISTORY.md
• PlexCleaner/ProcessFile.cs
• PlexCleanerTests/ReplaceOriginalTests.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 2b33763b51445ec293df6ec43ba9269a5fa7da4e, the content this pull request carries at that commit against develop, and it recorded 0 findings.

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

PR Summary by Qodo

Warn when HandBrake rewrites drop embedded closed captions

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

AI Description

• Warn before HandBrake deinterlace or repair when embedded captions will be lost or detection
 fails.
• Avoid redundant scans, sidecar mutations, and repair-error accounting; cancellation still stops
 the rewrite.
• Document caption loss and test detection, probe failures, and scan-skipping behavior.

Diagram

graph TD
  D["Deinterlace"] --> C["HandBrake conversion"] --> S{"Scan recorded?"} --> P["Isolated caption scan"] --> W["Caption warning"] --> H["HandBrake rewrite"] --> M["Subtitle remux"]
  R["Re-encode fallback"] --> C
  S -->|"yes"| H
  P -->|"no captions"| H

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Block rewrites when captions are detected

• ➕ Prevents known caption loss.
• ➖ Leaves affected media without the requested deinterlace or repair.

2. Extract and restore embedded captions

• ➕ Could preserve captions while completing the rewrite.
• ➖ Requires a substantially broader, codec-sensitive preservation workflow and validation.

Recommendation: Keep the warning-first approach for this fix: it makes the existing loss visible without changing rewrite behavior. Caption preservation merits separate work if retaining embedded captions becomes a requirement.

Files changed (4) +135 / -2

Bug fix (1) +55 / -2

ProcessFile.csDetect and report captions before shared HandBrake conversion +55/-2

Detect and report captions before shared HandBrake conversion

• Adds a pre-rewrite check used by deinterlace and the re-encode repair fallback. It skips scans already recorded by caption state, avoids writing new scan state, isolates probe tool errors from repair accounting, and stops on cancellation.

PlexCleaner/ProcessFile.cs

Tests (1) +75 / -0

ReplaceOriginalTests.csTest caption warnings and scan-state behavior +75/-0

Test caption warnings and scan-state behavior

• Covers captions present or absent, failed and signal-killed probes, unchanged scan state and outer tool-error count, and skipped probes when caption state is already recorded.

PlexCleanerTests/ReplaceOriginalTests.cs

Documentation (2) +5 / -0

ClosedCaptions.mdExplain caption loss during HandBrake rewrites +4/-0

Explain caption loss during HandBrake rewrites

• Documents why restoring subtitle tracks does not restore captions embedded in video. Describes the pre-rewrite warning and when an earlier scan avoids another scan.

Docs/ClosedCaptions.md

HISTORY.mdAdd the caption-loss warning to the release history +1/-0

Add the caption-loss warning to the release history

• Notes that HandBrake rewrites drop embedded captions and warn when those captions are detected, regardless of the removal setting.

HISTORY.md

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

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

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

The only remaining active finding is a nit in Docs/ClosedCaptions.md at line 190, where the first sentence does not clearly say when HandBrake rewrites video during deinterlacing and the paragraph omits the warning that a failed scan may drop Closed Captions.

1 open issue

• Clarify HandBrake deinterlacing and closed-caption warning behavior · New

What changed in this PR

This PR makes a known HandBrake side effect visible. HandBrake runs with no subtitles, and MkvMerge merges back only the original subtitle tracks. Embedded EIA-608/CTA-708 closed captions are part of the video stream, not a track, so a HandBrake rewrite drops them. The drop was already documented, but nothing reported it. The PR keeps the drop and adds a warning before each HandBrake run. It also documents the behavior in the history and closed captions docs.

Changes:

• ConvertUsingHandBrake() now calls a new ReportClosedCaptionsDropped() before the HandBrake run. It logs a warning when the video has captions, or an "unknown" warning when the scan fails. A failed scan never blocks the rewrite, and a cancelled scan returns.
• The check is skipped when the file already records a scan or a caption removal. It runs in its own tool error scope, so a failed caption scan does not count as a repair failure. It also does not write the scan state to the sidecar mid-rewrite.
• FindClosedCaptionTracks() gets a recordScan parameter (default true). The new check passes false.
• New tests cover captions present, absent, and a failed or killed scan. They also cover the skip when a scan or removal is already recorded.

File	Description
PlexCleanerTests/​ReplaceOriginalTests.cs	Adds tests for the caption warning cases, the unknown-scan case, and the skip when a scan is already recorded.
PlexCleaner/​ProcessFile.cs	Adds the closed caption check before HandBrake runs and the recordScan parameter that keeps the sidecar unchanged.
HISTORY.md	Adds a 3.23 entry noting that HandBrake drops embedded captions and that a warning is now logged.
Docs/​ClosedCaptions.md	Adds a "HandBrake Rewrites" section explaining the drop, the warning, and when the scan is skipped.

_{Review effort: Lite}

[ptr727]

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