Skip to content

fix(offline): show every track of server-prepared downloads - #550

Merged
Quick104 merged 2 commits into
Silo-Server:mainfrom
Rhainland:fix/offline-multitrack-downloads
Sep 29, 2026
Merged

Quick104 merged 2 commits into
Silo-Server:mainfrom
Rhainland:fix/offline-multitrack-downloads

Conversation

@Rhainland

@Rhainland Rhainland commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Related issue: Silo-Server/silo-server#1167
Validation tasks: changes #319 C1

Server-prepared downloads (remux or transcode) used to contain one audio track and no subtitles. With Silo-Server/silo-server#1681 they carry every audio track, keep SRT/WebVTT subtitles inside the MP4, and list ASS/SSA and PGS tracks as sidecar files in the offline manifest. On iOS those files played, but the audio menu showed generic names, the first subtitle track looked like a default, and PGS sidecars weren't recognised as bitmap subtitles.

Platforms: iOS and macOS, which share the offline download code. tvOS has no downloads.

Approach

  • For server-prepared downloads, audio tracks take their titles from the offline manifest by position when the track counts match. The engine doesn't read MP4 track titles. Subtitle tracks inside the MP4 lose the default flag the MP4 muxer puts on the first one, so only forced flags and the viewer's preferences turn subtitles on.
  • sup sidecars are classified, ranked and labelled as PGS. Sidecars show the manifest's track title, so "Full" and "Signs & Songs" ASS tracks are distinguishable.
  • An out-of-range saved audio position falls back to the file's default track instead of failing playback.

Original and older prepared downloads behave as before, apart from the sup classification and the audio fallback.

Validation

  • iOS, tvOS and macOS builds succeed. xcodebuild test on an iPhone 17 Pro simulator: 2221 tests, 0 failures, 2 skipped.
  • End to end against a local server running the server PR, with downloads played from the Downloads tab:
    • Multi-audio, multi-subtitle download: every audio track listed by title, audio switching works, subtitles start off, and SRT, ASS and PGS render.
    • An audio track picked on the detail page before downloading is the default offline.
    • Downloading with the version on Auto uses the default version; picking a specific version downloads that file.
    • A single-audio, single-subtitle title works.
    • Original-quality downloads are unchanged.
  • The macOS app builds, but I didn't exercise its UI.

Risks

Behaviour only changes when the manifest describes a server-prepared file. Requires the server PR for the extra tracks and sidecars; against an older server, downloads look as they did before.

Checklist

  • I read and can explain the complete diff.
  • This pull request addresses one concern.

AI Disclosure

  • Harness: Claude Code (Claude Agent SDK) in T3 Code
  • Tool(s): Claude Code; OpenAI Codex CLI 0.155.1 (review)
  • Model(s): claude-opus-5-5[1m]; gpt-6-astra (review)
  • Involvement: Fully AI-generated, human verified
  • Adversarial review: Claude Code's code-review and the Codex CLI reviewer (gpt-6-astra, medium effort) each reviewed this diff twice alongside the server and Android changes. They found no iOS defects. The cross-repo review found identical labels for multiple ASS sidecars, fixed here by showing the manifest title.

Note

Fix offline player to show every track of server-prepared downloads

  • Server-prepared downloads (non-original delivery format) are now detected via OfflineManifest.isServerPreparedFile in DownloadModels.swift and carried through playback into the player.
  • OfflinePreparedTrackInventory in OfflinePreparedTrackInventory.swift replaces MP4-muxer synthesized audio names with manifest titles and languages when audio counts match, and clears synthesized titles and default flags from embedded timed-text tracks.
  • External subtitle sidecars retain their manifest title and hearingImpaired flag; the classifier in SubtitleCodecClassifier.swift now treats sup as a bitmap (PGS) codec, including for display ordering and formatting.
  • Invalid offline audio ordinals now log a warning and fall back to the file's default audio stream instead of throwing invalidAudioTrackIndex.
  • Risk: when manifest and probed audio counts differ, synthesized titles are cleared but no manifest metadata is applied — check audioTracks in OfflinePreparedTrackInventory.swift for mismatch handling regressions.

Macroscope summarized 35c698a.

Server-prepared (remux/transcode) downloads now carry every audio track
and embedded subtitle. Label their audio from the offline manifest, ignore
the MP4 muxer's default flag on subtitle tracks, classify sup sidecars as
PGS, show sidecar titles, and fall back to the file's default track when a
saved audio position is out of range.
@kody-ai

kody-ai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Code Review Completed! 🔥

The code review was successfully completed based on your current configurations.

Kody Guide: Usage and Configuration
Interacting with Kody
  • Request a Review: Ask Kody to review your PR manually by adding a comment with the `@kody start-review` command at the root of your PR.

  • Provide Feedback: Help Kody learn and improve by reacting to its comments with a 👍 for helpful suggestions or a 👎 if improvements are needed.

Providing Context (Files & MCPs)

Add these hints in your PR description (or a comment) to unlock deeper checks:

  • Ticket / Acceptance Criteria: `Refs: ABC-123` (Linear/Jira/Asana/ClickUp/Trello) or a direct ticket link.
  • Bugfix Validation: a Sentry/Datadog/Bugsnag event link (or paste the stack trace/error message).
  • Endpoint Risk: mention the route (e.g., `POST /api/payments`) or controller/action name.
  • Attach a repo file as context: use an explicit marker like `@file:docs/guide.mdx#L10-L50` (replace with your real path).
  • API Contract Docs: include `@file:openapi.yaml` or `@file:swagger.json` when changing routes/schemas.
  • Definition of Done / Standards: include `@file:DOD.md` or `@file:CONTRIBUTING.md` if your repo has them.
  • Design System Source of Truth: include `@file:ui/index.ts` (replace with your DS entrypoint path).
  • Feature Flags: include the flag key/name and `@file:flags.ts` / `@file:config.json` (and optionally the PostHog flag name).
  • Edge/CDN Rules: link the Cloudflare rule/zone or describe the intended redirect/header behavior.
  • Attach an MCP tool output: use `@mcp<provider|tool>` (replace with an installed MCP provider + tool, e.g., `@mcp<sentry|events.search>`).
Current Kody Configuration
Review Options

The following review options are enabled or disabled:

Options Enabled
Bug ✅
Performance ✅
Security ✅
Business Logic ❌

Access your configuration settings here.

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

Next included review available in 47 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

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

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 8ad6a1d9-457d-4b85-8440-018075c81f35

📥 Commits

Reviewing files that changed from the base of the PR and between 23c924e and 35c698a.

📒 Files selected for processing (2)
  • iosApp/Tests/OfflinePreparedTrackInventoryTests.swift
  • iosApp/iosApp/Screens/Player/OfflinePreparedTrackInventory.swift

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: b563b418-0ca3-49c7-b3b4-b94941084648

📥 Commits

Reviewing files that changed from the base of the PR and between 4e7ed67 and 23c924e.

📒 Files selected for processing (11)
  • iosApp/Tests/OfflinePreparedTrackInventoryTests.swift
  • iosApp/Tests/SubtitleDisplayOrderTests.swift
  • iosApp/iosApp/Downloads/DownloadModels.swift
  • iosApp/iosApp/Downloads/OfflinePlayback.swift
  • iosApp/iosApp/Screens/Detail/DetailPlaybackFormatting.swift
  • iosApp/iosApp/Screens/Player/AetherLoadSpec.swift
  • iosApp/iosApp/Screens/Player/OfflinePreparedTrackInventory.swift
  • iosApp/iosApp/Screens/Player/PlayerTrack.swift
  • iosApp/iosApp/Screens/Player/PlayerViewModel.swift
  • iosApp/iosApp/Screens/Player/Subtitles/SubtitleCodecClassifier.swift
  • iosApp/iosApp/Screens/Player/Subtitles/SubtitleDisplayOrder.swift

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Offline playback now identifies server-prepared files, applies manifest metadata to prepared audio and subtitle inventories, resolves audio ordinals against probed streams, and recognizes sup sidecars as PGS.

Changes

Offline track playback

Layer / File(s) Summary
Prepared-file and sidecar metadata
iosApp/iosApp/Downloads/DownloadModels.swift, iosApp/iosApp/Downloads/OfflinePlayback.swift, iosApp/iosApp/Screens/Player/Subtitles/SubtitleCodecClassifier.swift, iosApp/iosApp/Screens/Detail/DetailPlaybackFormatting.swift, iosApp/iosApp/Screens/Player/Subtitles/SubtitleDisplayOrder.swift, iosApp/Tests/SubtitleDisplayOrderTests.swift
Manifests and prepared playback carry prepared-file status and subtitle titles. Subtitle codec classification and display ordering recognize sup as a PGS format.
Playback inventory and audio selection
iosApp/iosApp/Screens/Player/AetherLoadSpec.swift, iosApp/iosApp/Screens/Player/OfflinePreparedTrackInventory.swift, iosApp/iosApp/Screens/Player/PlayerTrack.swift, iosApp/iosApp/Screens/Player/PlayerViewModel.swift, iosApp/Tests/OfflinePreparedTrackInventoryTests.swift
Offline playback resolves audio ordinals against probed stream IDs and adjusts audio and subtitle inventories for prepared files. Tests cover prepared-file detection, inventory metadata, audio mapping, subtitle selection, and sidecar codecs.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant OfflineManifest
  participant OfflinePlayback
  participant PlayerViewModel
  participant AetherLoadSpec
  participant OfflinePreparedTrackInventory
  OfflineManifest->>OfflinePlayback: Provide prepared-file status and subtitle metadata
  OfflinePlayback->>PlayerViewModel: Provide prepared playback context
  PlayerViewModel->>AetherLoadSpec: Resolve manifest audio ordinal against probed IDs
  AetherLoadSpec-->>PlayerViewModel: Return stream ID or nil
  PlayerViewModel->>OfflinePreparedTrackInventory: Adjust prepared-file track inventory
Loading

Suggested reviewers: quick104

Merge Risk: ⚪ Minimal · up to 23c92

No identified playback issue currently blocks merging after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 23c92

The changes remain within offline playback and do not appear to add network access or privileges. The main remaining uncertainty is whether every server-prepared manifest lists audio tracks in the same order as the downloaded file.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed path is confined to playback of the user's downloaded media and sidecars. Inspected consumers do not show the prepared-file flag granting access to another asset, service, or data store.

Trust Boundaries and Controls

  • observed — The manifest can influence displayed titles and declared sidecar codec classification. The codec correction operates on the player's track inventory; sidecar registration separately passes the existing format hint to the playback engine.

Resilience and Maintainability Implications

  • observed — Bitmap-classified subtitles are rejected as secondary subtitles. The newly recognized sup format follows that restriction.

Hardening Proposals

  • proposed — Validate the server-produced manifest against the delivered file's audio ordering, not only its track count, before relying on positional titles and saved selections.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 32 functions across 11 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: showing all tracks for server-prepared offline downloads.
Description check ✅ Passed The description is directly related to the changeset. It explains the server-prepared download behavior, manifest-based track labeling, subtitle handling, PGS sidecars, fallback behavior, validation, …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

Comment @coderabbitai help to get the list of available commands.

Comment on lines +41 to +44
return track.replacing(
title: hasSynthesizedName(track) ? nil : track.title,
isDefault: false
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kody code-review Bug high

Unwanted subtitle playback persists because clearing isDefault does not undo Aether's muxer-default selection: when the offline manifest has no subtitle policy, the auto-resolver returns .noChange and adoptAetherInventory retains engine.activeSubtitleTrackIndex. Clear the engine's selection after the offline load when there is no explicit or automatic subtitle choice, before publishing the inventory.

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't happen. AetherEngine only activates a subtitle at load through LoadOptions.preferredSubtitleLanguages (AetherEngine+Subtitles.swift, the preferred-subtitle auto-select); a container's default disposition never selects one. Offline loads with no subtitle preference leave activeSubtitleTrackIndex nil, so adoptAetherInventory has nothing to keep. testMuxerDefaultDoesNotEnableSubtitlesOffline covers the resolver side. On an iOS simulator, prepared downloads whose first MP4 subtitle carries the muxer default started with subtitles off during cue windows. Clearing isDefault here only keeps the menu and later preference resolution from treating that flag as intent.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’re right. I conflated the muxer’s default disposition with subtitle activation. On an offline load without a preferred subtitle language, activeSubtitleTrackIndex remains nil, so the default flag does not turn subtitles on. The resolver test and simulator check support that distinction. Clearing isDefault here prevents the flag from being treated as intent in the menu or later preference resolution. I withdraw that concern.

Comment thread iosApp/iosApp/Screens/Player/OfflinePreparedTrackInventory.swift Outdated
@kody-ai

kody-ai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Code Review Completed! 🔥

The code review was successfully completed based on your current configurations.

Kody Guide: Usage and Configuration
Interacting with Kody
  • Request a Review: Ask Kody to review your PR manually by adding a comment with the `@kody start-review` command at the root of your PR.

  • Provide Feedback: Help Kody learn and improve by reacting to its comments with a 👍 for helpful suggestions or a 👎 if improvements are needed.

Providing Context (Files & MCPs)

Add these hints in your PR description (or a comment) to unlock deeper checks:

  • Ticket / Acceptance Criteria: `Refs: ABC-123` (Linear/Jira/Asana/ClickUp/Trello) or a direct ticket link.
  • Bugfix Validation: a Sentry/Datadog/Bugsnag event link (or paste the stack trace/error message).
  • Endpoint Risk: mention the route (e.g., `POST /api/payments`) or controller/action name.
  • Attach a repo file as context: use an explicit marker like `@file:docs/guide.mdx#L10-L50` (replace with your real path).
  • API Contract Docs: include `@file:openapi.yaml` or `@file:swagger.json` when changing routes/schemas.
  • Definition of Done / Standards: include `@file:DOD.md` or `@file:CONTRIBUTING.md` if your repo has them.
  • Design System Source of Truth: include `@file:ui/index.ts` (replace with your DS entrypoint path).
  • Feature Flags: include the flag key/name and `@file:flags.ts` / `@file:config.json` (and optionally the PostHog flag name).
  • Edge/CDN Rules: link the Cloudflare rule/zone or describe the intended redirect/header behavior.
  • Attach an MCP tool output: use `@mcp<provider|tool>` (replace with an installed MCP provider + tool, e.g., `@mcp<sentry|events.search>`).
Current Kody Configuration
Review Options

The following review options are enabled or disabled:

Options Enabled
Bug ✅
Performance ✅
Security ✅
Business Logic ❌

Access your configuration settings here.

@Quick104
Quick104 merged commit 1d03554 into Silo-Server:main Sep 29, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants