diff --git a/docs/IMPROVEMENTS.md b/docs/IMPROVEMENTS.md index 88bdd86..13e4d82 100644 --- a/docs/IMPROVEMENTS.md +++ b/docs/IMPROVEMENTS.md @@ -2,76 +2,60 @@ Track usability, DX, and architecture improvements. Focus: user-friendliness, easy setup, easy diagnosis. +Each entry below links to a dedicated page in [`docs/improvements/`](./improvements/) with full details. + +--- + ## Gitignore -- DB file `data/arm.db` is not gitignored — should be added so test/seed data isn't committed. +- [gitignore.md](./improvements/gitignore.md) — DB file `data/arm.db` not gitignored ## Data / Persistence -- **Migrate from `DateTime` to `DateTimeOffset`** — All model timestamps (`Job.StartTime`, `Job.StopTime`, `Notification.Timestamp`, `DiscMetadata.CreatedAt`/`LastUsedAt`, etc.) use `DateTime`. EF Core reads these back as `DateTimeKind.Unspecified` from SQLite, losing the UTC context. `.ToLocalTime()` in views works but is a band-aid. Switching to `DateTimeOffset` stores the offset with the value and makes timezone intent explicit. Requires touching models, EF mappings, all view comparisons, and serialization. +- [data-persistence.md](./improvements/data-persistence.md) — Migrate from `DateTime` to `DateTimeOffset` ## Configuration & Setup -- **Add 4K UHD disc type with separate settings** — Currently `ArmSettings` and `ConfigSnapshot` only have `HbArgsDvd` / `HbPresetDvd` and `HbArgsBd` / `HbPresetBd`. 4K UHD discs need different handling (ffmpeg passthrough to preserve HDR, no HandBrake re-encode). Add `HbArgsUhd` / `HbPresetUhd` and `FfmpegPostFileArgsUhd` properties, a `DiscType.Uhd` enum value, and wire them through `HandBrakeService.GetHbSettings()` and the `Conductor` pipeline. For 4K UHD, the typical workflow is `USE_FFMPEG: true` with `-c:v copy -c:a ac3 -b:a 640k` to preserve HDR metadata losslessly. Users currently have to swap `arm.yaml` manually when switching between 1080p and 4K discs — automatic disc-type detection would eliminate this. -- No `appsettings.Production.json` or env-aware config profiles. Would help users separate sensitive keys (API keys) from path config. -- Seed data scripts exist in `scripts/` but require manual run. Consider auto-seeding on first launch for demo/testing. +- [configuration-4k-uhd.md](./improvements/configuration-4k-uhd.md) — Add 4K UHD disc type with separate settings +- [configuration-appsettings.md](./improvements/configuration-appsettings.md) — Environment-aware config profiles +- [configuration-seed-data.md](./improvements/configuration-seed-data.md) — Auto-seed data on first launch + +## Disc Type Detection +- [disc-type-audiobook-mp3.md](./improvements/disc-type-audiobook-mp3.md) — MP3 audiobook / audio CD-ROM classification ## UI / User Experience -- **Restart from last successful stage** — add a "retry from failure" action that resumes the pipeline at the last failed stage instead of restarting from scratch. Requires each stage to checkpoint its completion state in the DB (e.g. a `Stages` table or bitfield on `Job`). +- [ui-restart-last-stage.md](./improvements/ui-restart-last-stage.md) — Restart from last successful stage ## SignalR -- `SignalRNotificationBroadcaster` is wired via `INotificationBroadcaster` interface. Works but the broadcaster is a singleton while the hub context is scoped per connection. Should verify no lifetime issues. +- [signalr.md](./improvements/signalr.md) — Broadcaster lifetime concerns ## Pages / Views -- **Redesign Identification section** — improve the layout of the Identification section on the Job detail page. -- **DVD/Blu-ray detection workflow** — the Settings page has a "Detect Disc" / "Scan Drives" button but the actual udev-based monitoring workflow from the original ARM isn't replicated. Should add a "Start Monitoring" action that runs the Conductor/IdentifyService loop. -- **Home dashboard** — core metrics displayed. Could add charts (job success rate over time, rips per day) or sparkline trends. -- Batch actions on Active Rips page (abandon all, retry all). +- [pages-identification.md](./improvements/pages-identification.md) — Redesign Identification section +- [pages-disc-detection.md](./improvements/pages-disc-detection.md) — DVD/Blu-ray detection workflow +- [pages-dashboard.md](./improvements/pages-dashboard.md) — Home dashboard enhancements +- [pages-batch-actions.md](./improvements/pages-batch-actions.md) — Batch actions on Active Rips page ## MusicBrainz -- **Investigate moving off XML where possible** — MusicBrainz XML parsing is fragile (manual XElement traversal, namespace handling). If MusicBrainz offers a JSON endpoint, prefer it. +- [musicbrainz.md](./improvements/musicbrainz.md) — Investigate moving off XML ## Dependency Injection -- WebUi now has full DI wiring. However many services are registered as `Scoped` when they're effectively stateless. `CliProcessRunner` is singleton. Review lifetime choices — some could be singletons or transient. +- [dependency-injection.md](./improvements/dependency-injection.md) — Review lifetime choices ## Startup & Recovery -- **Resume in-progress rips on restart** — currently, if the app is restarted while a job is ripping (VideoRipping, TranscodeActive, etc.), the background task is lost and the job stays stuck. On startup, scan for jobs in non-terminal states and resume them: re-attach the MakeMKV/HandBrake process if still running, or restart the rip/transcode stage from where it left off. Requires stage-level checkpointing (which stage completed, which files were produced) so the system can pick up without re-doing completed work. +- [startup-recovery.md](./improvements/startup-recovery.md) — Resume in-progress rips on restart ## Testing -- **Audio CD test:** Deferred — low priority. Needs abcde conf and audio disc in drive. -- **Data disc test:** Deferred — needs data disc for testing. -- **Error recovery tests:** Deferred — needs dirty/scratched discs for edge case testing. -- No CRC64 test with real DVD data (uses synthetic directory). -- No SignalR hub tests. +- [testing.md](./improvements/testing.md) — Deferred / missing tests ## Security -- `LogsController.Reader` uses `Path.GetFileName` for sanitization but could use `Path.GetFullPath` + prefix validation as defense-in-depth. +- [security.md](./improvements/security.md) — LogsController sanitization ## MCP (Model Context Protocol) -- ✅ **MCP server implemented** using the C# SDK (`ModelContextProtocol.AspNetCore` v1.4.1) with HTTP (Streamable HTTP) transport at `/mcp`. -- Exposed tools: - - `get_jobs` — list jobs with optional status filter, `offset`, and `limit` pagination. - - `get_logs` — read job log files with `offset` (line number) and `pageSize` for efficient browsing of long logs. - - `get_config` — returns current ARM Sharp configuration (API key presence is shown as booleans, values are never exposed). -- 🔲 Future tools: `update_config`, `eject_drive`, `trigger_identify`, log streaming via SSE. +- [mcp.md](./improvements/mcp.md) — Exposed tools & future tools ## Container / Deployment -- Docker image is ~2GB with full .NET SDK. Switch to self-contained publish with runtime-only image to reduce size. -- GitHub Actions CI has QEMU set up but only builds `linux/amd64`. Add `linux/arm64` multi-arch build once ARM64 runners or cross-compilation are available. -- **HandBrake nvdec support** — current `arm-dependencies:1.7.3` base image compiles HandBrake without `--enable-nvdec`. The devcontainer has a custom rebuild with nvdec working, but the production Dockerfile still uses the base image's build (no hw-decoding). Need to either fork and rebuild `arm-dependencies`, or add a multi-stage HandBrake build step to the production Dockerfile. -- Docker buildx warning — migrate from legacy builder to BuildKit. +- [container-deployment.md](./improvements/container-deployment.md) — Image size, multi-arch, nvdec, buildkit ## Disc Databases (Track Identification) -- **thediscdb.com integration** — Encrypted BDs often return 0 tracks from `makemkvcon info --robot`. thediscdb.com stores disc IDs mapped to known track layouts. Adding a lookup step would let us skip the expensive `makemkvcon info` scan for known discs and identify the correct main feature track without guessing by filesize. API is simple REST — define a `DiscDatabaseService` client, cache results locally, and plug into `IdentifyService`. +- [disc-databases.md](./improvements/disc-databases.md) — thediscdb.com integration ## Notifications (Low Priority) -Pushbullet, IFTTT, JSON webhook, and Bash script notifications are already implemented in `NotificationService.SendRemoteNotificationsAsync()`. Two additional channels remain: - -### Pushover -- **API:** `POST https://api.pushover.net/1/messages.json` with `token` (app key), `user` (user key), `message`, `title`, `sound`, etc. -- **Config keys:** `PoUserKey` / `PO_USER_KEY` already exist in `ArmSettings` and `ConfigSnapshot`, mapped from YAML. Missing: a `PoAppToken` key for the application token. -- **Implementation:** ~20 lines in `NotificationService` — `SendPushoverAsync(client, appToken, userKey, title, body, ct)`. -- **Settings UI:** Apprise tab currently read-only; would need editable form fields. - -### Apprise -- **CLI:** `apprise` is a command-line tool supporting 80+ notification services (Slack, Discord, Telegram, email, etc.). Original Python ARM invokes it via subprocess. -- **Config key:** `Apprise` / `APPRISE` already exist in `ArmSettings` and `ConfigSnapshot`. -- **Implementation:** ~30 lines in `NotificationService` — call `apprise -b "body" -t "title"` via `CliProcessRunner`. -- **Settings UI:** Same as Pushover — needs editable form on Apprise tab. +- [notifications-pushover.md](./improvements/notifications-pushover.md) — Pushover integration +- [notifications-apprise.md](./improvements/notifications-apprise.md) — Apprise integration diff --git a/docs/improvements/configuration-4k-uhd.md b/docs/improvements/configuration-4k-uhd.md new file mode 100644 index 0000000..5859fe1 --- /dev/null +++ b/docs/improvements/configuration-4k-uhd.md @@ -0,0 +1,7 @@ +# Configuration & Setup — Add 4K UHD Disc Type with Separate Settings + +Currently `ArmSettings` and `ConfigSnapshot` only have `HbArgsDvd` / `HbPresetDvd` and `HbArgsBd` / `HbPresetBd`. 4K UHD discs need different handling (ffmpeg passthrough to preserve HDR, no HandBrake re-encode). + +Add `HbArgsUhd` / `HbPresetUhd` and `FfmpegPostFileArgsUhd` properties, a `DiscType.Uhd` enum value, and wire them through `HandBrakeService.GetHbSettings()` and the `Conductor` pipeline. For 4K UHD, the typical workflow is `USE_FFMPEG: true` with `-c:v copy -c:a ac3 -b:a 640k` to preserve HDR metadata losslessly. + +Users currently have to swap `arm.yaml` manually when switching between 1080p and 4K discs — automatic disc-type detection would eliminate this. diff --git a/docs/improvements/configuration-appsettings.md b/docs/improvements/configuration-appsettings.md new file mode 100644 index 0000000..4e81e24 --- /dev/null +++ b/docs/improvements/configuration-appsettings.md @@ -0,0 +1,3 @@ +# Configuration & Setup — Environment-Aware Config Profiles + +No `appsettings.Production.json` or env-aware config profiles exists. Would help users separate sensitive keys (API keys) from path config. diff --git a/docs/improvements/configuration-seed-data.md b/docs/improvements/configuration-seed-data.md new file mode 100644 index 0000000..a567ec3 --- /dev/null +++ b/docs/improvements/configuration-seed-data.md @@ -0,0 +1,3 @@ +# Configuration & Setup — Auto-Seed Data + +Seed data scripts exist in `scripts/` but require manual run. Consider auto-seeding on first launch for demo/testing. diff --git a/docs/improvements/container-deployment.md b/docs/improvements/container-deployment.md new file mode 100644 index 0000000..9f321ce --- /dev/null +++ b/docs/improvements/container-deployment.md @@ -0,0 +1,13 @@ +# Container / Deployment + +## Docker Image Size +Docker image is ~2GB with full .NET SDK. Switch to self-contained publish with runtime-only image to reduce size. + +## Multi-Arch Builds +GitHub Actions CI has QEMU set up but only builds `linux/amd64`. Add `linux/arm64` multi-arch build once ARM64 runners or cross-compilation are available. + +## HandBrake nvdec Support +Current `arm-dependencies:1.7.3` base image compiles HandBrake without `--enable-nvdec`. The devcontainer has a custom rebuild with nvdec working, but the production Dockerfile still uses the base image's build (no hw-decoding). Need to either fork and rebuild `arm-dependencies`, or add a multi-stage HandBrake build step to the production Dockerfile. + +## BuildKit Migration +Docker buildx warning — migrate from legacy builder to BuildKit. diff --git a/docs/improvements/data-persistence.md b/docs/improvements/data-persistence.md new file mode 100644 index 0000000..d02f7c8 --- /dev/null +++ b/docs/improvements/data-persistence.md @@ -0,0 +1,5 @@ +# Data / Persistence — Migrate from `DateTime` to `DateTimeOffset` + +All model timestamps (`Job.StartTime`, `Job.StopTime`, `Notification.Timestamp`, `DiscMetadata.CreatedAt`/`LastUsedAt`, etc.) use `DateTime`. EF Core reads these back as `DateTimeKind.Unspecified` from SQLite, losing the UTC context. `.ToLocalTime()` in views works but is a band-aid. + +Switching to `DateTimeOffset` stores the offset with the value and makes timezone intent explicit. Requires touching models, EF mappings, all view comparisons, and serialization. diff --git a/docs/improvements/dependency-injection.md b/docs/improvements/dependency-injection.md new file mode 100644 index 0000000..b89c512 --- /dev/null +++ b/docs/improvements/dependency-injection.md @@ -0,0 +1,3 @@ +# Dependency Injection — Review Lifetime Choices + +WebUi now has full DI wiring. However many services are registered as `Scoped` when they're effectively stateless. `CliProcessRunner` is singleton. Review lifetime choices — some could be singletons or transient. diff --git a/docs/improvements/disc-databases.md b/docs/improvements/disc-databases.md new file mode 100644 index 0000000..7724871 --- /dev/null +++ b/docs/improvements/disc-databases.md @@ -0,0 +1,5 @@ +# Disc Databases — thediscdb.com Integration + +Encrypted BDs often return 0 tracks from `makemkvcon info --robot`. thediscdb.com stores disc IDs mapped to known track layouts. Adding a lookup step would let us skip the expensive `makemkvcon info` scan for known discs and identify the correct main feature track without guessing by filesize. + +API is simple REST — define a `DiscDatabaseService` client, cache results locally, and plug into `IdentifyService`. diff --git a/docs/improvements/disc-type-audiobook-mp3.md b/docs/improvements/disc-type-audiobook-mp3.md new file mode 100644 index 0000000..a4aa558 --- /dev/null +++ b/docs/improvements/disc-type-audiobook-mp3.md @@ -0,0 +1,34 @@ +# Disc Type Detection — MP3 Audiobook / Audio CD-ROM Support + +## Context + +Discovered while investigating job 607: an "Angels & Demons" audiobook disc mounted as an iso9660 filesystem containing `.mp3` files at the root — no `VIDEO_TS`, `BDMV`, or `CDA` directories present. + +## The Problem + +`GetDiscType()` in `IdentifyService.cs` only recognizes three disc structures: + +| Check | Pattern | Detects | +|-------|---------|---------| +| `Directory.Exists("VIDEO_TS")` | DVD | ✅ | +| `Directory.Exists("BDMV")` | Blu-ray | ✅ | +| `FindOnDisc("CDA")` | Audio CD | ✅ | +| *(everything else)* | `DiscType.Unknown` | ❌ Fails | + +An MP3 audiobook disc has none of these structures, so the Conductor hits the `default` case and the job fails with: + +> `Couldn't identify the disc type. Exiting without any action.` + +## What Should Happen + +The system already has a `DiscType.Data` case in the Conductor that calls `RipDataAsync`, and an existing `DiscType.Music` path using MusicBrainZ + `RipMusicAsync`. We should add MP3/audio file detection to `GetDiscType()`: + +- Scan the root of the mounted disc for common audio file extensions (`.mp3`, `.flac`, `.ogg`, `.wav`, `.m4a`, `.aac`) +- If audio files are found, classify as `DiscType.Music` so the existing music pipeline can handle it +- Alternatively, classify as `DiscType.Data` as a fallback for discs with files but no known video/audio directory structure + +## Open Questions + +- Should `RipMusicAsync` / the music pipeline handle MP3 files, or should this be a generic file copy (data disc) operation? +- Would MusicBrainZ identification work for an audiobook with no CDDA tracks? +- Should we add a new `DiscType.Audiobook` for dedicated handling? diff --git a/docs/improvements/gitignore.md b/docs/improvements/gitignore.md new file mode 100644 index 0000000..3b5bcb4 --- /dev/null +++ b/docs/improvements/gitignore.md @@ -0,0 +1,3 @@ +# Gitignore + +DB file `data/arm.db` is not gitignored — should be added so test/seed data isn't committed. diff --git a/docs/improvements/mcp.md b/docs/improvements/mcp.md new file mode 100644 index 0000000..8bc433c --- /dev/null +++ b/docs/improvements/mcp.md @@ -0,0 +1,16 @@ +# MCP (Model Context Protocol) + +✅ **MCP server implemented** using the C# SDK (`ModelContextProtocol.AspNetCore` v1.4.1) with HTTP (Streamable HTTP) transport at `/mcp`. + +## Exposed Tools + +- `get_jobs` — list jobs with optional status filter, `offset`, and `limit` pagination. +- `get_logs` — read job log files with `offset` (line number) and `pageSize` for efficient browsing of long logs. +- `get_config` — returns current ARM Sharp configuration (API key presence is shown as booleans, values are never exposed). + +## Future Tools (🔲) + +- `update_config` +- `eject_drive` +- `trigger_identify` +- Log streaming via SSE diff --git a/docs/improvements/musicbrainz.md b/docs/improvements/musicbrainz.md new file mode 100644 index 0000000..2f419d2 --- /dev/null +++ b/docs/improvements/musicbrainz.md @@ -0,0 +1,3 @@ +# MusicBrainz — Investigate Moving Off XML + +MusicBrainz XML parsing is fragile (manual XElement traversal, namespace handling). If MusicBrainz offers a JSON endpoint, prefer it. diff --git a/docs/improvements/notifications-apprise.md b/docs/improvements/notifications-apprise.md new file mode 100644 index 0000000..e13c4e2 --- /dev/null +++ b/docs/improvements/notifications-apprise.md @@ -0,0 +1,6 @@ +# Notifications — Apprise + +- **CLI:** `apprise` is a command-line tool supporting 80+ notification services (Slack, Discord, Telegram, email, etc.). Original Python ARM invokes it via subprocess. +- **Config key:** `Apprise` / `APPRISE` already exist in `ArmSettings` and `ConfigSnapshot`. +- **Implementation:** ~30 lines in `NotificationService` — call `apprise -b "body" -t "title"` via `CliProcessRunner`. +- **Settings UI:** Same as Pushover — needs editable form on Apprise tab. diff --git a/docs/improvements/notifications-pushover.md b/docs/improvements/notifications-pushover.md new file mode 100644 index 0000000..572bc61 --- /dev/null +++ b/docs/improvements/notifications-pushover.md @@ -0,0 +1,6 @@ +# Notifications — Pushover + +- **API:** `POST https://api.pushover.net/1/messages.json` with `token` (app key), `user` (user key), `message`, `title`, `sound`, etc. +- **Config keys:** `PoUserKey` / `PO_USER_KEY` already exist in `ArmSettings` and `ConfigSnapshot`, mapped from YAML. Missing: a `PoAppToken` key for the application token. +- **Implementation:** ~20 lines in `NotificationService` — `SendPushoverAsync(client, appToken, userKey, title, body, ct)`. +- **Settings UI:** Apprise tab currently read-only; would need editable form fields. diff --git a/docs/improvements/pages-batch-actions.md b/docs/improvements/pages-batch-actions.md new file mode 100644 index 0000000..73679ad --- /dev/null +++ b/docs/improvements/pages-batch-actions.md @@ -0,0 +1,3 @@ +# Pages / Views — Batch Actions on Active Rips Page + +Add batch actions (abandon all, retry all) on the Active Rips page. diff --git a/docs/improvements/pages-dashboard.md b/docs/improvements/pages-dashboard.md new file mode 100644 index 0000000..c483e5e --- /dev/null +++ b/docs/improvements/pages-dashboard.md @@ -0,0 +1,3 @@ +# Pages / Views — Home Dashboard Enhancements + +Core metrics displayed. Could add charts (job success rate over time, rips per day) or sparkline trends. diff --git a/docs/improvements/pages-disc-detection.md b/docs/improvements/pages-disc-detection.md new file mode 100644 index 0000000..388888d --- /dev/null +++ b/docs/improvements/pages-disc-detection.md @@ -0,0 +1,3 @@ +# Pages / Views — DVD/Blu-ray Detection Workflow + +The Settings page has a "Detect Disc" / "Scan Drives" button but the actual udev-based monitoring workflow from the original ARM isn't replicated. Should add a "Start Monitoring" action that runs the Conductor/IdentifyService loop. diff --git a/docs/improvements/pages-identification.md b/docs/improvements/pages-identification.md new file mode 100644 index 0000000..71c4e5d --- /dev/null +++ b/docs/improvements/pages-identification.md @@ -0,0 +1,3 @@ +# Pages / Views — Redesign Identification Section + +Improve the layout of the Identification section on the Job detail page. diff --git a/docs/improvements/security.md b/docs/improvements/security.md new file mode 100644 index 0000000..eb433f3 --- /dev/null +++ b/docs/improvements/security.md @@ -0,0 +1,3 @@ +# Security — LogsController Sanitization + +`LogsController.Reader` uses `Path.GetFileName` for sanitization but could use `Path.GetFullPath` + prefix validation as defense-in-depth. diff --git a/docs/improvements/signalr.md b/docs/improvements/signalr.md new file mode 100644 index 0000000..0424812 --- /dev/null +++ b/docs/improvements/signalr.md @@ -0,0 +1,3 @@ +# SignalR — Broadcaster Lifetime Concerns + +`SignalRNotificationBroadcaster` is wired via `INotificationBroadcaster` interface. Works but the broadcaster is a singleton while the hub context is scoped per connection. Should verify no lifetime issues. diff --git a/docs/improvements/startup-recovery.md b/docs/improvements/startup-recovery.md new file mode 100644 index 0000000..3f21b8f --- /dev/null +++ b/docs/improvements/startup-recovery.md @@ -0,0 +1,3 @@ +# Startup & Recovery — Resume In-Progress Rips on Restart + +Currently, if the app is restarted while a job is ripping (VideoRipping, TranscodeActive, etc.), the background task is lost and the job stays stuck. On startup, scan for jobs in non-terminal states and resume them: re-attach the MakeMKV/HandBrake process if still running, or restart the rip/transcode stage from where it left off. Requires stage-level checkpointing (which stage completed, which files were produced) so the system can pick up without re-doing completed work. diff --git a/docs/improvements/testing.md b/docs/improvements/testing.md new file mode 100644 index 0000000..c02a43a --- /dev/null +++ b/docs/improvements/testing.md @@ -0,0 +1,7 @@ +# Testing — Deferred / Missing Tests + +- **Audio CD test:** Deferred — low priority. Needs abcde conf and audio disc in drive. +- **Data disc test:** Deferred — needs data disc for testing. +- **Error recovery tests:** Deferred — needs dirty/scratched discs for edge case testing. +- No CRC64 test with real DVD data (uses synthetic directory). +- No SignalR hub tests. diff --git a/docs/improvements/ui-restart-last-stage.md b/docs/improvements/ui-restart-last-stage.md new file mode 100644 index 0000000..0342a94 --- /dev/null +++ b/docs/improvements/ui-restart-last-stage.md @@ -0,0 +1,3 @@ +# UI / User Experience — Restart from Last Successful Stage + +Add a "retry from failure" action that resumes the pipeline at the last failed stage instead of restarting from scratch. Requires each stage to checkpoint its completion state in the DB (e.g. a `Stages` table or bitfield on `Job`). diff --git a/src/ArmRipper.Core/Rip/ArmRipperService.cs b/src/ArmRipper.Core/Rip/ArmRipperService.cs index 1023f58..df86928 100644 --- a/src/ArmRipper.Core/Rip/ArmRipperService.cs +++ b/src/ArmRipper.Core/Rip/ArmRipperService.cs @@ -198,8 +198,7 @@ public async Task RipVisualMediaAsync(Job job, string logFile, bool hasD Directory.CreateDirectory(makeMkvOutPath); var mkvArgs = job.Config?.MkvArgs ?? settings.Value.MkvArgs ?? ""; - var minLength = job.Config?.MinLength ?? settings.Value.MinLength; - await makeMkv.RipTrackAsync(job, "0", makeMkvOutPath, mkvArgs, minLength, MkvProgress(job, "Ripping track 0", ct), ct); + await makeMkv.RipTrackAsync(job, "0", makeMkvOutPath, mkvArgs, 0, MkvProgress(job, "Ripping track 0", ct), ct); logger.LogInformation("Ripped track 0 in test mode"); return makeMkvOutPath; } @@ -210,9 +209,9 @@ public async Task RipVisualMediaAsync(Job job, string logFile, bool hasD var minLengthCfg = config?.MinLength ?? settings.Value.MinLength; var maxLength = config?.MaxLength ?? settings.Value.MaxLength; - // When DiscDb is enabled, pass infoMinLength=0 so MakeMKV reports ALL tracks, - // including short extras that may match DiscDb entries. Our own minLengthCfg - // and DiscDb promotion logic will handle filtering and promotion. + // Use infoMinLength=0 when DiscDb is enabled so MakeMKV reports ALL tracks, + // including short extras that may match DiscDb entries. The normal + // minLengthCfg is only used for the rip phase, not the scan. var infoMinLength = settings.Value.DiscDbEnabled ? 0 : (int?)null; var tracks = await makeMkv.GetTrackInfoWithCacheAsync(job, jobTitle, infoMinLength, ct); @@ -227,28 +226,51 @@ public async Task RipVisualMediaAsync(Job job, string logFile, bool hasD await db.SaveChangesAsync(ct); await BroadcastJobUpdateAsync(job); - if (!Directory.Exists(makeMkvOutPath)) - Directory.CreateDirectory(makeMkvOutPath); - - var mkvArgs = config?.MkvArgs ?? settings.Value.MkvArgs ?? ""; - await makeMkv.RipAllTitlesAsync(job, makeMkvOutPath, mkvArgs, minLengthCfg, MkvProgress(job, "Ripping all titles", ct), ct); - logger.LogInformation("Ripped all titles from disc (0-track fallback)"); + // The info scan may have timed out with infoMinLength=0 on a + // damaged disc. Before falling back to RipAllTitles, try a second + // info scan with the normal configured minLength. If that succeeds, + // the normal track selection (MainFeature, etc.) will be applied. + // This prevents an identify-phase timeout from cascading into a rip + // that bypasses track selection and rips everything. + var retryTracks = await makeMkv.GetTrackInfoWithCacheAsync(job, jobTitle, + infoMinLength: null, ct); - if (!Directory.EnumerateFileSystemEntries(makeMkvOutPath).Any()) + if (retryTracks.Count > 0) { - var msg = "MakeMKV rip produced no output files"; - logger.LogError(msg); - throw new InvalidOperationException(msg); + tracks = retryTracks; + logger.LogInformation( + "0-track fallback: retry with normal minLength found {Count} tracks, " + + "proceeding with standard track selection", retryTracks.Count); } - - if (job.Config?.NotifyRip ?? settings.Value.NotifyRip) + else { - await notifications.NotifyAsync(job, NotificationService.NotifyTitle, - $"{job.Title} rip complete. Starting transcode.", ct); - } + if (!Directory.Exists(makeMkvOutPath)) + Directory.CreateDirectory(makeMkvOutPath); - logger.LogInformation("************* Ripping with MakeMKV completed *************"); - return makeMkvOutPath; + var mkvArgs = config?.MkvArgs ?? settings.Value.MkvArgs ?? ""; + await makeMkv.RipAllTitlesAsync(job, makeMkvOutPath, mkvArgs, minLengthCfg, MkvProgress(job, "Ripping all titles", ct), ct); + logger.LogInformation("Ripped all titles from disc (0-track fallback)"); + + if (!Directory.EnumerateFileSystemEntries(makeMkvOutPath).Any()) + { + var msg = "MakeMKV rip produced no output files"; + logger.LogError(msg); + throw new InvalidOperationException(msg); + } + + job.MarkStageComplete(RipStage.Rip); + await db.SaveChangesAsync(ct); + await BroadcastJobUpdateAsync(job); + + if (job.Config?.NotifyRip ?? settings.Value.NotifyRip) + { + await notifications.NotifyAsync(job, NotificationService.NotifyTitle, + $"{job.Title} rip complete. Starting transcode.", ct); + } + + logger.LogInformation("************* Ripping with MakeMKV completed *************"); + return makeMkvOutPath; + } } Track? longestTrack = null; @@ -414,9 +436,9 @@ await notifications.NotifyAsync(job, NotificationService.NotifyTitle, { var firstTrack = eligibleTracks.FirstOrDefault(); if (firstTrack is not null) - await makeMkv.RipTrackAsync(job, firstTrack.TrackNumber!, makeMkvOutPath, mkvArgs, minLengthCfg, MkvProgress(job, "Ripping track 0", ct), ct); + await makeMkv.RipTrackAsync(job, firstTrack.TrackNumber!, makeMkvOutPath, mkvArgs, 0, MkvProgress(job, "Ripping track 0", ct), ct); else - await makeMkv.RipTrackAsync(job, "0", makeMkvOutPath, mkvArgs, minLengthCfg, MkvProgress(job, "Ripping track 0", ct), ct); + await makeMkv.RipTrackAsync(job, "0", makeMkvOutPath, mkvArgs, 0, MkvProgress(job, "Ripping track 0", ct), ct); } else if (config?.MainFeature ?? settings.Value.MainFeature) { @@ -426,7 +448,9 @@ await notifications.NotifyAsync(job, NotificationService.NotifyTitle, var main = tracks.FirstOrDefault(t => t.MainFeature); if (main is not null) { - await makeMkv.RipTrackAsync(job, main.TrackNumber!, makeMkvOutPath, mkvArgs, minLengthCfg, MkvProgress(job, "Ripping main feature", ct), ct); + // We already identified the exact track (the longest one), so pass + // minLength=0 to prevent MakeMKV from filtering it out with --minlength. + await makeMkv.RipTrackAsync(job, main.TrackNumber!, makeMkvOutPath, mkvArgs, 0, MkvProgress(job, "Ripping main feature", ct), ct); ripCount = 1; } } diff --git a/src/ArmRipper.Core/Rip/FfmpegService.cs b/src/ArmRipper.Core/Rip/FfmpegService.cs index dd7d727..922ed6f 100644 --- a/src/ArmRipper.Core/Rip/FfmpegService.cs +++ b/src/ArmRipper.Core/Rip/FfmpegService.cs @@ -249,7 +249,8 @@ public async Task TranscodeAllAsync(Job job, string rawPath, string o private async Task RunTranscodeAsync(string inputFile, string outputFile, Job job, int? totalSeconds, List stdOut, List stdErr, IProgress? progress, CancellationToken ct) { - await using var slot = await transcodeSlotLimiter.AcquireAsync(ct); + var effectiveMax = job.Config?.MaxConcurrentTranscodes ?? settings.Value.MaxConcurrentTranscodes; + await using var slot = await transcodeSlotLimiter.AcquireAsync(effectiveMax, ct); var (ffPreArgs, ffPostArgs) = GetFfSettings(job); diff --git a/src/ArmRipper.Core/Rip/HandBrakeService.cs b/src/ArmRipper.Core/Rip/HandBrakeService.cs index 5622bd1..dda2b81 100644 --- a/src/ArmRipper.Core/Rip/HandBrakeService.cs +++ b/src/ArmRipper.Core/Rip/HandBrakeService.cs @@ -42,7 +42,8 @@ public async Task TranscodeMkvAsync(Job job, string rawPath, string o logger.LogInformation("Transcoding {File} to {Output}", file, outputFile); var cmd = BuildCommand(file, outputFile, job, trackNumber: null, mainFeature: false); - lastResult = await RunHandBrakeCommandAsync(cmd, ct, progress); + var effectiveMax = job.Config?.MaxConcurrentTranscodes ?? settings.Value.MaxConcurrentTranscodes; + lastResult = await RunHandBrakeCommandAsync(cmd, effectiveMax, ct, progress); if (lastResult.ExitCode != 0) { @@ -118,10 +119,11 @@ public async Task TranscodeMainFeatureAsync(Job job, string rawPath, logger.LogInformation("Ripping main feature to {Output}", outputFile); var cmd = BuildCommand(rawPath, outputFile, job, trackNumber: null, mainFeature: true); + var effectiveMax = job.Config?.MaxConcurrentTranscodes ?? settings.Value.MaxConcurrentTranscodes; try { - var result = await RunHandBrakeCommandAsync(cmd, ct, progress); + var result = await RunHandBrakeCommandAsync(cmd, effectiveMax, ct, progress); if (result.ExitCode != 0) { var err = $"HandBrake main feature transcoding failed with code {result.ExitCode}: {result.StdErr}"; @@ -191,10 +193,11 @@ public async Task TranscodeAllAsync(Job job, string rawPath, string o logger.LogInformation("Transcoding title {TrackNo} to {Output}", trackNo, outputFile); var cmd = BuildCommand(rawPath, outputFile, job, trackNo, mainFeature: false); + var effectiveMax = job.Config?.MaxConcurrentTranscodes ?? settings.Value.MaxConcurrentTranscodes; try { - lastResult = await RunHandBrakeCommandAsync(cmd, ct, progress); + lastResult = await RunHandBrakeCommandAsync(cmd, effectiveMax, ct, progress); if (lastResult.ExitCode != 0) { var err = $"HandBrake encoding of title {trackNo} failed with code {lastResult.ExitCode}: {lastResult.StdErr}"; @@ -277,9 +280,9 @@ private string BuildCommand(string inputPath, string outputPath, Job job, int? t return (settings.Value.HbPresetDvd, settings.Value.HbArgsDvd); } - private async Task RunHandBrakeCommandAsync(string cmd, CancellationToken ct, IProgress? progress = null) + private async Task RunHandBrakeCommandAsync(string cmd, int maxConcurrent, CancellationToken ct, IProgress? progress = null) { - await using var slot = await transcodeSlotLimiter.AcquireAsync(ct); + await using var slot = await transcodeSlotLimiter.AcquireAsync(maxConcurrent, ct); logger.LogInformation("HandBrake command: {Command}", cmd); diff --git a/src/ArmRipper.Core/Rip/ITranscodeSlotLimiter.cs b/src/ArmRipper.Core/Rip/ITranscodeSlotLimiter.cs index fba0ac7..cf5d7da 100644 --- a/src/ArmRipper.Core/Rip/ITranscodeSlotLimiter.cs +++ b/src/ArmRipper.Core/Rip/ITranscodeSlotLimiter.cs @@ -2,5 +2,10 @@ namespace ArmRipper.Core.Rip; public interface ITranscodeSlotLimiter { - ValueTask AcquireAsync(CancellationToken ct = default); + /// + /// Acquires a transcode slot, respecting the effective max-concurrent limit. + /// + /// Maximum concurrent transcodes allowed. + /// Use 0 or negative to disable limiting entirely. + ValueTask AcquireAsync(int maxConcurrent, CancellationToken ct = default); } diff --git a/src/ArmRipper.Core/Rip/MakeMkvService.cs b/src/ArmRipper.Core/Rip/MakeMkvService.cs index aa63813..b1c1ee6 100644 --- a/src/ArmRipper.Core/Rip/MakeMkvService.cs +++ b/src/ArmRipper.Core/Rip/MakeMkvService.cs @@ -409,10 +409,10 @@ private sealed record StreamAccum public async Task RipTrackAsync(Job job, string trackNumber, string outputPath, string mkvArgs, int minLength, IProgress? progress = null, CancellationToken ct = default) { - // Estimate expected file size from track info for progress monitoring + // Estimate expected file size from the track for progress monitoring. var expectedSize = job.Tracks - .Where(t => t.TrackNumber == trackNumber) - .Sum(t => t.FileSize ?? 0); + ?.FirstOrDefault(t => t.TrackNumber == trackNumber) + ?.FileSize ?? 0; var monitorCts = CancellationTokenSource.CreateLinkedTokenSource(ct); var monitorTask = expectedSize > 0 && progress is not null @@ -421,6 +421,11 @@ public async Task RipTrackAsync(Job job, string trackNumber, string outputPath, try { + // trackNumber is the 0-based TINFO index from MakeMKV's info scan. + // MakeMKV's mkv command uses TINFO indices: 0 = "all titles", + // 1 = first title, etc. SourceTitleId (field 24) is a different + // numbering scheme and must NOT be used here — it would select the + // wrong title on almost every disc. var args = $"--robot --messages=-stdout --progress=-stdout mkv --minlength={minLength} dev:{job.DevPath} {trackNumber} \"{outputPath}\""; if (!string.IsNullOrEmpty(mkvArgs)) args = $"--robot --messages=-stdout --progress=-stdout mkv {mkvArgs} --minlength={minLength} dev:{job.DevPath} {trackNumber} \"{outputPath}\""; diff --git a/src/ArmRipper.Core/Rip/TranscodeSlotLimiter.cs b/src/ArmRipper.Core/Rip/TranscodeSlotLimiter.cs index f0fe7b4..d2a0384 100644 --- a/src/ArmRipper.Core/Rip/TranscodeSlotLimiter.cs +++ b/src/ArmRipper.Core/Rip/TranscodeSlotLimiter.cs @@ -1,37 +1,108 @@ -using ArmRipper.Core.Configuration; -using Microsoft.Extensions.Options; - namespace ArmRipper.Core.Rip; +/// +/// Global concurrency gate for transcode processes. Unlike a fixed +/// , this limiter reads the effective +/// MaxConcurrentTranscodes from each caller at acquire time so +/// that UI-driven settings changes take effect without a restart. +/// public sealed class TranscodeSlotLimiter : ITranscodeSlotLimiter { - private readonly SemaphoreSlim? semaphore; + private readonly object _gate = new(); + private int _activeCount; + private readonly Queue _waitQueue = new(); - public TranscodeSlotLimiter(IOptions settings) + public ValueTask AcquireAsync(int maxConcurrent, CancellationToken ct = default) { - var max = settings.Value.MaxConcurrentTranscodes; - if (max > 0) - semaphore = new SemaphoreSlim(max, max); + // 0 or negative → no limiting + if (maxConcurrent <= 0) + return new ValueTask(NoopLease.Instance); + + lock (_gate) + { + if (_activeCount < maxConcurrent) + { + _activeCount++; + return new ValueTask(new Lease(this)); + } + + var waiter = new Waiter(); + _waitQueue.Enqueue(waiter); + + // Register cancellation: when the token fires, mark the waiter + // as done so ReleaseOne skips it. If ReleaseOne already claimed + // it, TrySetCanceled is a no-op. + if (ct.CanBeCanceled) + { + waiter.Cancellation = ct.Register(static state => + { + var w = (Waiter)state!; + // Atomically mark as done; if ReleaseOne hasn't claimed it yet, cancel the TCS. + if (Interlocked.Exchange(ref w.Done, 1) == 0) + w.Tcs.TrySetCanceled(); + }, waiter); + } + + return new ValueTask(waiter.Tcs.Task); + } } - public async ValueTask AcquireAsync(CancellationToken ct = default) + /// Called by when a transcode finishes. + private void ReleaseOne() { - if (semaphore is null) - return NoopLease.Instance; + while (true) + { + Waiter? next = null; + lock (_gate) + { + while (_waitQueue.Count > 0) + { + var w = _waitQueue.Dequeue(); + // Try to claim this waiter before it's cancelled + if (Interlocked.Exchange(ref w.Done, 1) == 0) + { + next = w; + break; + } + // Already cancelled — clean up its registration + w.Cancellation.Dispose(); + } - await semaphore.WaitAsync(ct); - return new SemaphoreLease(semaphore); + if (next is null) + { + // No waiters — just release the slot + _activeCount--; + return; + } + } + + // Dispose the cancellation registration now that we own the waiter + next.Cancellation.Dispose(); + + if (next.Tcs.TrySetResult(new Lease(this))) + return; + // If TrySetResult failed (extremely unlikely race), loop and try next waiter + } } - private sealed class SemaphoreLease(SemaphoreSlim semaphore) : IAsyncDisposable + private sealed class Waiter { - private readonly SemaphoreSlim semaphore = semaphore; - private int disposed; + public TaskCompletionSource Tcs { get; } = new(TaskCreationOptions.RunContinuationsAsynchronously); + public CancellationTokenRegistration Cancellation; + /// + /// 0 = waiting, 1 = done (either fulfilled by ReleaseOne or cancelled). + /// + public int Done; + } + + private sealed class Lease(TranscodeSlotLimiter limiter) : IAsyncDisposable + { + private int _disposed; public ValueTask DisposeAsync() { - if (Interlocked.Exchange(ref disposed, 1) == 0) - semaphore.Release(); + if (Interlocked.Exchange(ref _disposed, 1) == 0) + limiter.ReleaseOne(); return ValueTask.CompletedTask; } } @@ -39,7 +110,6 @@ public ValueTask DisposeAsync() private sealed class NoopLease : IAsyncDisposable { public static readonly NoopLease Instance = new(); - public ValueTask DisposeAsync() => ValueTask.CompletedTask; } } diff --git a/src/ArmRipper.WebUi/Views/Completed/Detail.cshtml b/src/ArmRipper.WebUi/Views/Completed/Detail.cshtml index 7ecb595..f529d6a 100644 --- a/src/ArmRipper.WebUi/Views/Completed/Detail.cshtml +++ b/src/ArmRipper.WebUi/Views/Completed/Detail.cshtml @@ -12,7 +12,13 @@
-
@Model.FileName
+
+ @Model.FileName + @if (Model.SizeBytes < 2_000_000_000 && Model.RelativeDirectory.StartsWith("movies", StringComparison.OrdinalIgnoreCase)) + { + ⚠️ Small file + } +
← Back
diff --git a/src/ArmRipper.WebUi/Views/Completed/Index.cshtml b/src/ArmRipper.WebUi/Views/Completed/Index.cshtml index f6eed81..18733bd 100644 --- a/src/ArmRipper.WebUi/Views/Completed/Index.cshtml +++ b/src/ArmRipper.WebUi/Views/Completed/Index.cshtml @@ -147,6 +147,10 @@ else @file.FileName + @if (file.SizeBytes < 2_000_000_000 && file.RelativeDirectory.StartsWith("movies", StringComparison.OrdinalIgnoreCase)) + { + ⚠️ Small file + } @file.LastModifiedFormatted @file.SizeFormatted diff --git a/src/ArmRipper.WebUi/Views/Jobs/JobDetail.cshtml b/src/ArmRipper.WebUi/Views/Jobs/JobDetail.cshtml index 7aa4e89..19faab9 100644 --- a/src/ArmRipper.WebUi/Views/Jobs/JobDetail.cshtml +++ b/src/ArmRipper.WebUi/Views/Jobs/JobDetail.cshtml @@ -394,11 +394,15 @@ else {
- + - @foreach (var track in Model.Tracks.OrderBy(t => t.TrackNumber)) - { + @{ + var orderedTracks = Model.Tracks.OrderBy(t => t.TrackNumber).ToList(); + for (var idx = 0; idx < orderedTracks.Count; idx++) + { + var track = orderedTracks[idx]; + + } }
#TitleLengthChaptersSizeFPSAspectSourceTypeRippedMainStatus
#MKV #TitleLengthChaptersSizeFPSAspectSourceTypeRippedMainStatus
@(idx + 1) @track.TrackNumber @if (!string.IsNullOrEmpty(track.EpisodeTitle)) @@ -445,6 +449,7 @@ else @(track.MainFeature ? "Yes" : "") @(track.Status ?? "—")