-
Notifications
You must be signed in to change notification settings - Fork 75
Seekable streams (Safari/iOS fix for video) #580
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ent caching (same as images)
📝 WalkthroughWalkthroughGetVideoAsset in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Logic as PooledImmichFrameLogic
participant API as Immich API
participant Disk as Filesystem
Caller->>Logic: Request GetVideoAsset(id)
Logic->>Disk: compute path "id.mp4" + check DownloadImages
alt DownloadImages = true
Logic->>Disk: ensure directory exists
Disk-->>Logic: dir ready
Logic->>Disk: file exists & check age
alt file fresh
Disk-->>Logic: return file stream
Logic-->>Caller: stream (disk-backed)
else expired or missing
Logic->>API: fetch video bytes
API-->>Logic: response (body, Content-Type?)
Logic->>Disk: write bytes to id.mp4
Disk-->>Logic: file ready
Logic-->>Caller: stream (disk-backed)
end
else DownloadImages = false
Logic->>API: fetch video bytes
API-->>Logic: response (body, Content-Type?)
Logic->>Logic: default Content-Type -> "video/mp4" if missing
Logic-->>Caller: MemoryStream with bytes
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Around line 165-206: In GetVideoAsset (PooledImmichFrameLogic.GetVideoAsset)
move the local cache check (using _downloadLocation, fileName and
_generalSettings.RenewImagesDuration) to run before calling
_immichApi.PlayAssetVideoAsync so we short‑circuit on cache hits; then call the
API only if cache miss or expired, wrap the returned videoResponse (and its
Stream) in a using block to ensure disposal, and on API failure fall back to
returning the cached file if present; mirror the flow used in GetImageAsset to
ensure no leaked streams and proper cache-first behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Around line 176-180: The cache-hit branch in PooledImmichFrameLogic is
returning a hardcoded content-type "video/mp4" for cached files (see filePath,
fileName and the caller that expects a tuple of (fileName, contentType,
Stream)), which can mismatch original responses; fix by persisting the original
content-type when first saving the file (e.g., write a companion metadata file
next to filePath like filePath + ".meta" or embed a suffix) and on cache hit
read that metadata and return the stored content-type instead of the hardcoded
"video/mp4"; also add a safe fallback to "video/mp4" if the metadata is missing
or unreadable and ensure any write/read logic is added where the file is created
and where the cached file is returned.
🧹 Nitpick comments (1)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
190-192: Consider extracting Content-Type parsing to reduce duplication.The Content-Type extraction logic is duplicated across both branches. A small helper could improve maintainability.
♻️ Optional refactor
+ private static string GetContentType(FileResponse response, string defaultType) + { + return response.Headers.ContainsKey("Content-Type") + ? response.Headers["Content-Type"].FirstOrDefault() ?? defaultType + : defaultType; + } + private async Task<(string fileName, string ContentType, Stream fileStream)> GetVideoAsset(Guid id) { // ... cache check ... using var videoResponse = await _immichApi.PlayAssetVideoAsync(id, string.Empty); if (videoResponse == null) throw new AssetNotFoundException($"Video asset {id} was not found!"); - var contentType = videoResponse.Headers.ContainsKey("Content-Type") - ? videoResponse.Headers["Content-Type"].FirstOrDefault() ?? "video/mp4" - : "video/mp4"; + var contentType = GetContentType(videoResponse, "video/mp4");Also applies to: 208-210
|
Workflow is not running, I don't know why. |
Fix Safari/iOS video playback by buffering videos to seekable streams
Safari requires seekable streams for video range requests. Previously, non-seekable network streams from Immich caused playback failures(error code 4 - MEDIA_ERR_SRC_NOT_SUPPORTED). Now videos are either cached to disk (when DownloadImages=true) or buffered to memory to provide seekability.
Summary by CodeRabbit
Bug Fixes
Performance