prepare for transcoding support#2580
Closed
gtsteffaniak wants to merge 15 commits into
Closed
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a full on-demand HLS transcode pipeline: backend ffmpeg probing, HLS segment/init handlers with in-memory session store, stream byte-range budget enforcement, and Plyr-only frontend player with transcode-offer flow, scrub-preview popup, and seek-on-release utilities. Removes the ChangesMedia playback and transcode pipeline
Sequence Diagram(s)sequenceDiagram
participant Browser
participant PlyrViewer as plyrViewer.vue
participant ResourcesApi as resourcesApi
participant TranscodeSessionsHandler as GET /media/transcode/sessions
participant HLSPlaylistHandler as GET /media/transcode/hls/playlist.m3u8
participant TranscodeSessionStore
participant FfmpegService as ffmpeg.Service
PlyrViewer->>ResourcesApi: fetchTranscodeSessions(source, path)
ResourcesApi->>TranscodeSessionsHandler: GET sessions
TranscodeSessionsHandler->>TranscodeSessionStore: evaluate()
TranscodeSessionStore-->>PlyrViewer: CanStart + block reason
alt CanStart
PlyrViewer->>ResourcesApi: acquireTranscodeHLSSession(playlistUrl)
ResourcesApi->>HLSPlaylistHandler: GET playlist.m3u8
HLSPlaylistHandler->>TranscodeSessionStore: acquireHLS()
HLSPlaylistHandler->>FfmpegService: ProbeFile + BuildHLSSegmentParams
HLSPlaylistHandler-->>ResourcesApi: playlist + X-Transcode-Session
ResourcesApi-->>PlyrViewer: blobUrl + sessionId + hlsConfig
PlyrViewer->>Browser: hls.js attaches blobUrl
loop every 30s
PlyrViewer->>ResourcesApi: pingTranscodeSession(playhead)
end
PlyrViewer->>ResourcesApi: releaseTranscodeSession(source, path)
ResourcesApi->>TranscodeSessionStore: releaseForUserFile()
else blocked
PlyrViewer->>Browser: show transcode-offer panel
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Owner
Author
|
im actually not happy with this PR how it works right now. it always has to transcode the entire file for each viewing before serving. Will leave it here, but this needs to be fixed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/common/settings/structs.go (1)
397-398: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep
preview.autoplayMediatri-state for migration.This deprecated field now needs to preserve “unset” separately from explicit
false. WithSetDefaults()already seedingfileViewer.autoplayMediatotrue, an older config that setspreview.autoplayMedia: falsewill silently flip back to autoplay-on during upgrade. Make this field a*booland migrate when it is non-nil so explicit opt-outs survive.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/common/settings/structs.go` around lines 397 - 398, The deprecated preview autoplay setting currently uses a plain bool, which loses the distinction between “unset” and an explicit false during migration. Update the `AutoplayMedia` field in the settings struct to a `*bool`, then adjust the migration logic in the related settings/defaulting code so it only copies into `fileViewer.autoplayMedia` when the pointer is non-nil; keep `SetDefaults()` seeding the new location to true so explicit opt-outs from older configs survive upgrade.backend/http/stream_grant_test.go (1)
95-115: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCover the non-streamable branch too.
This update now only verifies the happy path. Since
backend/http/stream_grant.goadded a new “skip non-audio/video items” branch, please keep a non-media child in this fixture (or add a sibling case) and assert that it does not receive a token. As per path instructions,**/*.go: ensure functional features include new unit tests where its easily possible.Suggested coverage
file := &iteminfo.ExtendedFileInfo{ FileInfo: iteminfo.FileInfo{ ItemInfo: iteminfo.ItemInfo{Type: "directory"}, Files: []iteminfo.ExtendedItemInfo{ {ItemInfo: iteminfo.ItemInfo{Name: "clip.mp4", Type: "video/mp4"}}, + {ItemInfo: iteminfo.ItemInfo{Name: "poster.jpg", Type: "image/jpeg"}}, {ItemInfo: iteminfo.ItemInfo{Name: "nested", Type: "directory"}}, }, }, } @@ - if file.Files[1].StreamToken != "" { - t.Fatal("did not expect stream token on directory child folder") + if file.Files[1].StreamToken != "" { + t.Fatal("did not expect stream token on non-media child") + } + if file.Files[2].StreamToken != "" { + t.Fatal("did not expect stream token on directory child folder") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/http/stream_grant_test.go` around lines 95 - 115, The current TestAttachStreamTokensForDirectory only covers token assignment for streamable media; extend it to exercise the new skip-non-audio/video branch in attachStreamTokensForDirectory by including a non-streamable child in the fixture or adding a sibling assertion. Use the existing test helper and ExtendedFileInfo/ExtendedItemInfo setup to verify the media item still gets a StreamToken while the non-media item remains without one.Source: Path instructions
🧹 Nitpick comments (3)
backend/common/settings/config.go (1)
339-347: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the transcode defaults into shared constants.
2,"veryfast", and1080are now hard-coded in bothsetupMedia()andSetDefaults(). A shared constant/helper would keep runtime fallback and generated defaults from drifting. As per path instructions, "reduce duplication where there is clear duplication found, use consolidated functions."Also applies to: 1172-1174
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/common/settings/config.go` around lines 339 - 347, The transcode fallback values are duplicated between SetDefaults and setupMedia, so hoist the MaxConcurrent, Preset, and MaxResolution defaults into shared constants or a small helper and use them from both places. Update the Config.Integrations.Media.Transcode initialization logic to reference the shared values instead of hard-coded 2, "veryfast", and 1080 so runtime fallback and generated defaults stay aligned.Source: Path instructions
backend/http/transcode_test.go (1)
113-146: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a traversal rejection test for
/api/media/transcode.These handler tests cover missing token,
Range, and multi-file, but they never exercise theutils.SanitizePathbranch inbackend/http/transcode.go. Afile=../a.mkvcase would give direct regression coverage for the path traversal boundary on this new API.Suggested test
+func TestTranscodeHandlerRejectsTraversal(t *testing.T) { + t.Parallel() + d := &requestContext{user: &users.User{ID: 1}} + req := httptest.NewRequest(http.MethodGet, "/api/media/transcode?source=default&file=../a.mkv&streamToken=tok", nil) + status, err := transcodeHandler(httptest.NewRecorder(), req, d) + if status != http.StatusBadRequest || err == nil { + t.Fatalf("expected 400 for invalid path, got status=%d err=%v", status, err) + } +}As per path instructions, "ensure functional features include new unit tests where its easily possible" and "make sure all http apis have proper sanitization for path to prevent path traversal vulnerabilities."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/http/transcode_test.go` around lines 113 - 146, Add a new unit test in transcode_test.go to cover path traversal rejection for transcodeHandler. The current tests only check missing token, Range, and multi-file, so add a case using a file value like ../a.mkv to exercise the utils.SanitizePath path in backend/http/transcode.go. Verify transcodeHandler returns the forbidden response and an error for the traversal attempt, alongside the existing requestContext and mintStreamGrant setup used by the other tests.Source: Path instructions
frontend/src/utils/canBrowserPlayNative.test.js (1)
5-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the capability tests deterministic.
Several cases currently allow almost any outcome, so a regression in the native-vs-transcode decision can still pass. Stub
document.createElement('video').canPlayTypeandMediaSource.isTypeSupportedper scenario, then assert the exact expected result for each branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/utils/canBrowserPlayNative.test.js` around lines 5 - 37, The capability tests are too permissive and can hide regressions in native-vs-transcode decisions. In canBrowserPlayNative.test.js, stub document.createElement('video').canPlayType and MediaSource.isTypeSupported for each scenario so the environment is deterministic. Then update the assertions around canBrowserPlayNative and needsTranscodeFirst to expect one exact result per branch instead of allowing multiple outcomes. Keep the coverage focused on the existing canBrowserPlayNative and needsTranscodeFirst cases for mkv, mp4, and mime/codec combinations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/common/settings/config.go`:
- Around line 339-347: The transcode preset is only defaulted when empty, so
invalid values can still pass startup and fail later; update the
validation/defaulting in Config.SetDefaults for
Config.Integrations.Media.Transcode.Preset to reject unsupported values or
normalize them to the central default “veryfast”, and remove any duplicate
preset defaulting elsewhere so the behavior is defined in one place.
In `@backend/ffmpeg/transcode.go`:
- Around line 24-29: The probe cache key in ProbeFile is too coarse because
fileInfo.ModTime().Unix() drops sub-second changes and can reuse stale probe
results after rapid file replacements. Update the cache key generation near
os.Stat/path lookup to use higher-resolution file metadata from
fileInfo.ModTime() (or another sub-second-precise attribute) so quick rewrites
produce distinct keys and cache entries don’t mask new codec/container info.
In `@backend/http/stream_range.go`:
- Around line 136-140: The 416 response path in stream range handling is
incorrectly reusing the normal file headers, which sets a full-file
Content-Length and can confuse clients. Update the error branch in the stream
playback/window logic around applyStreamPlaybackWindow and
setStreamResponseHeaders so the 416 response only sends the Content-Range header
and does not include the full-file Content-Length; keep the rest of the response
metadata consistent with an unsatisfiable range response.
In `@backend/http/stream_window.go`:
- Around line 143-152: `streamWindowEntryFor` currently creates and retains a
`streamPlaybackWindow` forever in `streamWindowStore`, so stale `(sessionId,
streamToken)` entries can accumulate over the process lifetime. Add TTL-based
eviction and delete expired entries during lookup/creation in
`streamWindowEntryFor`, and apply the same cleanup behavior in the related
playback-window paths referenced by the review so expired windows are removed
instead of being kept indefinitely.
In `@backend/http/stream.go`:
- Around line 162-175: The public media stream handler advertises only
403/404/500, but `publicStreamHandler` also returns a 400 when
`utils.SanitizePath` rejects the `file` query. Update the Swagger annotations
for `publicStreamHandler` to include the invalid-path 400 response, keeping the
documented API contract aligned with the handler behavior.
In `@backend/http/transcode_sessions_test.go`:
- Line 18: The transcode session tests are mutating shared global configuration
via settings.Config.Integrations.Media.Transcode.MaxConcurrent, which can leak
state across tests. Add a small test helper (for example,
setTestTranscodeMaxConcurrent) that saves the original value, sets the
test-specific value, and restores it with t.Cleanup. Replace each direct
MaxConcurrent assignment in the transcode session test cases with that helper so
test order or shuffle does not affect later tests.
In `@backend/http/transcode_test.go`:
- Around line 66-99: The TestTranscodeTargetVideoKbps test is mutating shared
global config while running in parallel, which leaks state into other tests.
Move the MaxResolution override to a test-local setup/teardown in
TestTranscodeTargetVideoKbps, and restore
settings.Config.Integrations.Media.Transcode after the test finishes; keep the
t.Parallel usage only if the shared config is no longer being mutated, and
reference transcodeTargetVideoKbps and
settings.Config.Integrations.Media.Transcode.MaxResolution when updating the
test.
In `@backend/http/transcode.go`:
- Around line 239-247: Gate both transcode endpoints with the Realtime
permission before any session lookup or ffmpeg work. In transcodeSessionsHandler
and the other transcode handler at the same request flow, add an authorization
check on d.user.Permissions.Realtime alongside the existing TranscodeEnabled and
admin checks, and return a forbidden error if the user does not have Realtime
access. Keep the guard near the start of each handler so unauthorized users
cannot list or start transcodes.
In `@frontend/src/utils/canBrowserPlayNative.js`:
- Around line 50-58: The fallback in containerMimeFromFileName should not assume
an unknown file is video/mp4 when the extension is unrecognized and mimeType is
absent. Update containerMimeFromFileName and the codec check path it feeds
(including the related logic around canBrowserPlayNative) so unknown containers
are treated as unsupported unless a real container MIME is available, returning
a falsey/unknown result instead of forcing MP4.
In `@frontend/src/utils/fmp4MsePlayer.js`:
- Around line 21-24: The startup path in the MSE player leaves the MediaSource
object URL unreleased when initialization fails after
URL.createObjectURL(mediaSource) succeeds. Update the fmp4MsePlayer setup flow
around mediaSource/objectUrl, addSourceBuffer, and the fetch/body validation
paths so any early throw or non-OK response revokes the created object URL
before exiting, including the failure cases referenced by the later startup
branches.
- Around line 68-76: The pump logic in fmp4MsePlayer drops a chunk before append
success because queue.shift() is called inside pump() prior to
sourceBuffer.appendBuffer(). Update pump() so it only removes the fragment from
queue after appendBuffer succeeds, and on synchronous failure keep the same
chunk queued for retry rather than discarding it; use the existing pump, queue,
and sourceBuffer symbols to locate the change.
In `@frontend/src/utils/htmlPreview.test.js`:
- Around line 8-13: The URL mock in htmlPreview.test.js is hiding regressions
because getApiPath and getPublicApiPath ignore the route argument. Update the
mocked helpers to preserve the passed-in _route in the generated URL while still
encoding the file and inline parameters, so buildPreviewResourceUrl is forced to
use the correct route and the not.toContain('/media/stream') assertion remains
meaningful.
In `@frontend/src/views/files/plyrViewer.vue`:
- Around line 913-938: The async fire-and-forget calls in plyrViewer.vue are
triggering no-floating-promises because onVideoLoadedMetadata and
onVideoPlaybackError invoke offerTranscodePlayback() without handling the
returned promise, and the same pattern also appears in the related
startTranscodePlayback call site. Update these handlers to explicitly mark the
calls as intentionally unawaited with void, or convert the handlers to async and
await the promise where appropriate, using the existing method names
offerTranscodePlayback and startTranscodePlayback to locate the affected spots.
- Around line 895-911: Restore a visible fallback state in plyrViewer.vue when
transcode startup fails outside the 409/503 limit path: the catch around the
transcode playback flow currently only logs and clears useMsePlayback, which can
leave the component mounted but uninitialized. Update the error handling in the
transcode start logic so that non-limit failures also transition to the existing
offer/fallback UI state used by the component, likely by setting the same
transcode-related flags and/or emitting the appropriate event from plyrViewer’s
playback setup path. Keep the current special handling for
resourcesApi.fetchTranscodeSessions and the needs-transcode emission, but ensure
the parent/user sees a recoverable playback state after any startup failure.
In `@frontend/src/views/files/Preview.vue`:
- Around line 47-50: The Preview.vue session label currently renders raw
parentheses around sess.source, which triggers the i18n raw-text lint rule.
Update the template in the videoTranscodeOffer.sessions loop so the parentheses
are moved into an interpolation or localized string rather than hardcoded text,
keeping the sess.fileName/sess.path and sess.source display intact.
In `@frontend/src/views/files/ThreeJs.vue`:
- Line 367: The URL rewrite guard in ThreeJs.vue is missing the legacy
/api/resources/stream? endpoint, so textures still using that upstream contract
will no longer be handled. Update the condition around the stream URL check in
the ThreeJs URL handling logic to include the legacy stream path alongside the
existing media stream and download paths, keeping the rewrite behavior
compatible until all producers migrate.
---
Outside diff comments:
In `@backend/common/settings/structs.go`:
- Around line 397-398: The deprecated preview autoplay setting currently uses a
plain bool, which loses the distinction between “unset” and an explicit false
during migration. Update the `AutoplayMedia` field in the settings struct to a
`*bool`, then adjust the migration logic in the related settings/defaulting code
so it only copies into `fileViewer.autoplayMedia` when the pointer is non-nil;
keep `SetDefaults()` seeding the new location to true so explicit opt-outs from
older configs survive upgrade.
In `@backend/http/stream_grant_test.go`:
- Around line 95-115: The current TestAttachStreamTokensForDirectory only covers
token assignment for streamable media; extend it to exercise the new
skip-non-audio/video branch in attachStreamTokensForDirectory by including a
non-streamable child in the fixture or adding a sibling assertion. Use the
existing test helper and ExtendedFileInfo/ExtendedItemInfo setup to verify the
media item still gets a StreamToken while the non-media item remains without
one.
---
Nitpick comments:
In `@backend/common/settings/config.go`:
- Around line 339-347: The transcode fallback values are duplicated between
SetDefaults and setupMedia, so hoist the MaxConcurrent, Preset, and
MaxResolution defaults into shared constants or a small helper and use them from
both places. Update the Config.Integrations.Media.Transcode initialization logic
to reference the shared values instead of hard-coded 2, "veryfast", and 1080 so
runtime fallback and generated defaults stay aligned.
In `@backend/http/transcode_test.go`:
- Around line 113-146: Add a new unit test in transcode_test.go to cover path
traversal rejection for transcodeHandler. The current tests only check missing
token, Range, and multi-file, so add a case using a file value like ../a.mkv to
exercise the utils.SanitizePath path in backend/http/transcode.go. Verify
transcodeHandler returns the forbidden response and an error for the traversal
attempt, alongside the existing requestContext and mintStreamGrant setup used by
the other tests.
In `@frontend/src/utils/canBrowserPlayNative.test.js`:
- Around line 5-37: The capability tests are too permissive and can hide
regressions in native-vs-transcode decisions. In canBrowserPlayNative.test.js,
stub document.createElement('video').canPlayType and MediaSource.isTypeSupported
for each scenario so the environment is deterministic. Then update the
assertions around canBrowserPlayNative and needsTranscodeFirst to expect one
exact result per branch instead of allowing multiple outcomes. Keep the coverage
focused on the existing canBrowserPlayNative and needsTranscodeFirst cases for
mkv, mp4, and mime/codec combinations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ea42913a-5dcf-4f01-aa82-7b8b5fddc58b
⛔ Files ignored due to path filters (30)
backend/go.sumis excluded by!**/*.sumbackend/swagger/docs/docs.gois excluded by!backend/swagger/**backend/swagger/docs/swagger.jsonis excluded by!backend/swagger/**backend/swagger/docs/swagger.yamlis excluded by!backend/swagger/**frontend/src/i18n/ar.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/cz.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/de.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/el.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/en.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/es.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/fr.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/he.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/hu.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/is.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/it.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ja.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ko.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/nl-be.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/nl.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/pl.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/pt-br.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/pt.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ro.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ru.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/sk.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/sv-se.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/tr.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ua.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/zh-cn.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/zh-tw.jsonis excluded by!frontend/src/i18n/**
📒 Files selected for processing (44)
CHANGELOG.mdbackend/adapters/fs/files/files.gobackend/common/settings/apply_user_defaults_test.gobackend/common/settings/config.gobackend/common/settings/configFunctions.gobackend/common/settings/migration_test.gobackend/common/settings/settings.gobackend/common/settings/structs.gobackend/config.playwright.yamlbackend/database/users/users.gobackend/ffmpeg/cache.gobackend/ffmpeg/transcode.gobackend/go.modbackend/http/httpRouter.gobackend/http/middleware.gobackend/http/static.gobackend/http/stream.gobackend/http/stream_grant.gobackend/http/stream_grant_test.gobackend/http/stream_range.gobackend/http/stream_window.gobackend/http/stream_window_test.gobackend/http/transcode.gobackend/http/transcode_sessions.gobackend/http/transcode_sessions_test.gobackend/http/transcode_test.gobackend/indexing/iteminfo/fileinfo.gofrontend/public/config.generated.yamlfrontend/src/api/resources.jsfrontend/src/store/getters.jsfrontend/src/store/mutations.jsfrontend/src/store/types.tsfrontend/src/utils/canBrowserPlayNative.jsfrontend/src/utils/canBrowserPlayNative.test.jsfrontend/src/utils/fmp4MsePlayer.jsfrontend/src/utils/htmlPreview.test.jsfrontend/src/utils/htmlPreview.tsfrontend/src/utils/streamWindowSync.jsfrontend/src/views/files/DocViewer.vuefrontend/src/views/files/EpubViewer.vuefrontend/src/views/files/Preview.vuefrontend/src/views/files/ThreeJs.vuefrontend/src/views/files/plyrViewer.vuefrontend/src/views/settings/Profile.vue
💤 Files with no reviewable changes (8)
- backend/common/settings/settings.go
- frontend/src/store/types.ts
- backend/common/settings/migration_test.go
- backend/database/users/users.go
- backend/config.playwright.yaml
- frontend/src/views/settings/Profile.vue
- frontend/src/store/getters.js
- backend/common/settings/apply_user_defaults_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/utils/plyrScrubPreview.js`:
- Around line 343-359: The scrubbing cleanup in onScrubEnd only runs when the
seek input itself receives the release, so dragging off the control can leave
the preview and listeners active. Update the scrubbing flow in
plyrScrubPreview.js by wiring document-level release events alongside the
existing document mousemove/touchmove handlers in onScrubStart, and make them
call onScrubEnd when the pointer/touch ends anywhere. Ensure the added listeners
are removed in onScrubEnd together with the existing move listeners so the
popup, timers, and abortController always clean up.
- Around line 300-310: The scrub preview fetch path in
queueFetch()/fetchPreview() marks a bucket as fetched too early by setting
lastFetchedPercent before the image is actually cached. Move that state update
so it only happens after fetchPreview() successfully loads and caches the image,
and leave aborted or failed requests eligible to retry. Keep the change
localized around queueFetch, fetchPreview, and the lastFetchedPercent /
pendingPercent checks so the same percent can be fetched again if no cached
preview exists.
In `@frontend/src/utils/plyrSeekOnRelease.js`:
- Around line 50-67: The release handling in plyrSeekOnRelease currently dedupes
with lastCommitAt and event.timeStamp, which still allows commitPlyrSeek to run
twice when a mouseup/touchend is followed by change. Update the onCommit flow in
plyrSeekOnRelease so that a release event sets a one-shot flag to suppress the
immediately following change event, and reset that flag after it is consumed;
keep the existing capture listeners on mouseup, touchend, and change, but make
sure commitPlyrSeek is only invoked once per seek release.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 380b74c9-c1ec-427e-ad75-46506dd511b2
⛔ Files ignored due to path filters (3)
backend/swagger/docs/docs.gois excluded by!backend/swagger/**backend/swagger/docs/swagger.jsonis excluded by!backend/swagger/**backend/swagger/docs/swagger.yamlis excluded by!backend/swagger/**
📒 Files selected for processing (7)
backend/http/stream_range.gofrontend/src/api/resources.jsfrontend/src/utils/plyrScrubPreview.jsfrontend/src/utils/plyrScrubPreview.test.jsfrontend/src/utils/plyrSeekOnRelease.jsfrontend/src/utils/plyrSeekOnRelease.test.jsfrontend/src/views/files/plyrViewer.vue
💤 Files with no reviewable changes (2)
- backend/http/stream_range.go
- frontend/src/api/resources.js
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/views/files/Preview.vue (2)
229-230: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t load download-backed URLs in automatic viewers without a permission gate.
getOpenFileURLis backed by the download endpoint, but these branches feed automatic inline viewers. For users/shares with view access but no download access, Safari HEIC/HEIF and PDF previews can render as broken if the backend rejects the URL. Gate these branches onpermissions.download, or keep them on a view/preview endpoint that is valid for preview-only access.Also applies to: 249-250
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/views/files/Preview.vue` around lines 229 - 230, Gate the Safari HEIC/HEIF and PDF preview branches in Preview.vue before calling resourcesApi.getOpenFileURL, since that endpoint is download-backed and can fail for preview-only access. Update the logic in the Preview component so the automatic viewer paths only use getOpenFileURL when permissions.download is true, otherwise fall back to a view/preview-safe URL or skip the inline viewer flow. Apply the same check to both the isSafari/isHeicOrHeif branch and the PDF preview branch so shares with view-only permissions do not render broken previews.
54-57: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHide the Start button when the session check already denied starting.
When
status.canStartis false with any block reason other thanuser_limitorsystem_limit,modestays"offer", so Line 54 still shows the Start Transcode button and repeated clicks will re-check a denied action.Proposed fix
- let mode = 'offer'; - if (!status.canStart) { - if (status.blockReason === 'user_limit' || status.blockReason === 'system_limit') { - mode = status.blockReason; - } - } + const canStart = status?.canStart === true; + let mode = canStart ? 'offer' : status?.blockReason; + if (mode !== 'offer' && mode !== 'user_limit' && mode !== 'system_limit') { + mode = 'unsupported'; + } this.transcodePlaybackActive = false; this.videoTranscodeOffer = { mode, + canStart, sessions: status.sessions || [], systemLimit: status.systemLimit, userLimit: status.userLimit, };- v-if="videoTranscodeOffer.mode === 'offer'" + v-if="videoTranscodeOffer.canStart"Also applies to: 519-531
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/views/files/Preview.vue` around lines 54 - 57, The Start Transcode button is still rendered in Preview.vue when videoTranscodeOffer.mode remains "offer" even after a denied session check, so update the button visibility condition around startVideoTranscode/videoTranscodeOffer to also require that the current status allows starting. Use the existing status.canStart and its block reason handling so that only user_limit or system_limit keep the offer visible; for any other denial, hide the Start button and prevent repeated re-checks. Apply the same gating consistently in the related offer rendering block referenced by the same videoTranscodeOffer state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontend/src/views/files/Preview.vue`:
- Around line 229-230: Gate the Safari HEIC/HEIF and PDF preview branches in
Preview.vue before calling resourcesApi.getOpenFileURL, since that endpoint is
download-backed and can fail for preview-only access. Update the logic in the
Preview component so the automatic viewer paths only use getOpenFileURL when
permissions.download is true, otherwise fall back to a view/preview-safe URL or
skip the inline viewer flow. Apply the same check to both the
isSafari/isHeicOrHeif branch and the PDF preview branch so shares with view-only
permissions do not render broken previews.
- Around line 54-57: The Start Transcode button is still rendered in Preview.vue
when videoTranscodeOffer.mode remains "offer" even after a denied session check,
so update the button visibility condition around
startVideoTranscode/videoTranscodeOffer to also require that the current status
allows starting. Use the existing status.canStart and its block reason handling
so that only user_limit or system_limit keep the offer visible; for any other
denial, hide the Start button and prevent repeated re-checks. Apply the same
gating consistently in the related offer rendering block referenced by the same
videoTranscodeOffer state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3570ed73-776c-46e5-80f9-3c02a5a525f7
⛔ Files ignored due to path filters (28)
backend/swagger/docs/docs.gois excluded by!backend/swagger/**backend/swagger/docs/swagger.jsonis excluded by!backend/swagger/**backend/swagger/docs/swagger.yamlis excluded by!backend/swagger/**frontend/src/i18n/ar.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/cz.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/de.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/el.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/en.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/es.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/fr.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/he.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/hu.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/it.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ja.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ko.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/nl-be.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/nl.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/pl.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/pt-br.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/pt.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ro.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ru.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/sk.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/sv-se.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/tr.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ua.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/zh-cn.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/zh-tw.jsonis excluded by!frontend/src/i18n/**
📒 Files selected for processing (16)
CHANGELOG.mdbackend/common/settings/config.gobackend/ffmpeg/transcode.gobackend/http/stream.gobackend/http/transcode.gobackend/http/transcode_sessions_test.gobackend/http/transcode_test.gofrontend/src/store/mutations.jsfrontend/src/utils/canBrowserPlayNative.jsfrontend/src/utils/fmp4MsePlayer.jsfrontend/src/utils/htmlPreview.test.jsfrontend/src/utils/plyrScrubPreview.jsfrontend/src/utils/plyrSeekOnRelease.jsfrontend/src/views/files/Preview.vuefrontend/src/views/files/ThreeJs.vuefrontend/src/views/files/plyrViewer.vue
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (13)
- frontend/src/utils/plyrSeekOnRelease.js
- frontend/src/views/files/ThreeJs.vue
- backend/http/transcode_test.go
- frontend/src/utils/canBrowserPlayNative.js
- backend/http/transcode_sessions_test.go
- frontend/src/utils/htmlPreview.test.js
- backend/common/settings/config.go
- frontend/src/store/mutations.js
- frontend/src/utils/fmp4MsePlayer.js
- backend/ffmpeg/transcode.go
- backend/http/transcode.go
- backend/http/stream.go
- frontend/src/views/files/plyrViewer.vue
Collaborator
|
btw, I haven't tried the branch, but the scrubbing are frames of the video while seeking/touching the progress bar, or is another thing? cuz I think that plyr has one built-in but needs to be feed with images of the frames + timestamps, idk how would work, but mentioning just in case. (edit: yup, are preview thumbnails) By reading the summary sounds awesome! |
Owner
Author
|
thanks for pointing that out i didn't realize it had an option for that. It works really well the way its implemented here, but ill test out the plyr supported option too to see if thats better. |
Owner
Author
|
so that explains why some videos downloaded have that sprite sheet for thumbnails, its so plyr and things can have thumbnails. But for how filebrowser works its a much better feature without it. because the preview api already supports showing thumbnails for videos at any point in the video. It works very well. And no need for sprites |
Collaborator
|
I just tried it and yep, works well and looks nice! :)
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/http/stream_grant_test.go (1)
15-29: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd one round-trip test for the new grant metadata.
These updates widen
mintStreamGrant(...), but every case still passes0, nil, so the newly addedFileSize/DurationSecpayload is still unexercised. Please add a non-zero mint/validate case so regressions in the new transcode grant contract fail fast. As per path instructions, "ensure functional features include new unit tests where its easily possible".Also applies to: 32-43, 45-54, 75-92
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/http/stream_grant_test.go` around lines 15 - 29, Add a round-trip unit test for the new grant metadata by updating one of the existing mint/validate paths in stream_grant_test.go to call mintStreamGrant with non-zero FileSize and DurationSec values, then assert validateStreamGrant accepts the token for the same requestContext and payload shape. Use the existing mintStreamGrant and validateStreamGrant test flow so the new transcode grant contract is exercised and regressions in the FileSize/DurationSec payload fail fast.Source: Path instructions
backend/preview/preview.go (1)
176-210: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude
preserveAspectRatioin the preview cache key.
CacheKeystill ignores the new resize mode, so a cached square preview and a cached scrub preview now collide for the same file/size/seek tuple. Whichever request warms the cache first will leak the wrong geometry to later callers.Suggested fix
func GetPreviewForFile(ctx context.Context, file iteminfo.ExtendedFileInfo, previewSize, url string, seekPercentage int, preserveAspectRatio bool) ([]byte, error) { @@ - cacheKey := CacheKey(cacheHash, previewSize, seekPercentage) + cacheKey := CacheKey(cacheHash, previewSize, seekPercentage, preserveAspectRatio) @@ func GeneratePreviewWithMD5(ctx context.Context, file iteminfo.ExtendedFileInfo, previewSize, officeUrl string, seekPercentage int, fileMD5 string, preserveAspectRatio bool) ([]byte, error) { @@ - _, _ = hasher.Write([]byte(CacheKey(fileMD5, previewSize, seekPercentage))) + _, _ = hasher.Write([]byte(CacheKey(fileMD5, previewSize, seekPercentage, preserveAspectRatio))) @@ - cacheKey := CacheKey(fileMD5, previewSize, seekPercentage) + cacheKey := CacheKey(fileMD5, previewSize, seekPercentage, preserveAspectRatio)-func CacheKey(md5, previewSize string, percentage int) string { - key := fmt.Sprintf("%x%x%x", md5, previewSize, percentage) +func CacheKey(md5, previewSize string, percentage int, preserveAspectRatio bool) string { + key := fmt.Sprintf("%x%x%x:%t", md5, previewSize, percentage, preserveAspectRatio) return key }Also applies to: 403-505, 551-551
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/preview/preview.go` around lines 176 - 210, The preview cache key in GetPreviewForFile still omits preserveAspectRatio, so square and non-square previews can collide for the same file/size/seek values. Update the cache-key construction (the CacheKey call and any matching key generation in related preview paths such as GeneratePreviewWithMD5 and other referenced preview helpers) to include preserveAspectRatio so each resize mode stores and reads distinct cached data.frontend/src/utils/plyrScrubPreview.js (1)
409-419: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear loading when the active preview fetch fails.
If the last queued preview request fails while the pointer stays still, no image
errorevent fires and the loading indicator can remain stuck. Clear loading when the failed percent is still pending.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/utils/plyrScrubPreview.js` around lines 409 - 419, The scrubbing preview fetch in plyrScrubPreview.js leaves loading stuck when the currently pending request fails without triggering an image error. Update the try/catch around fetchPreviewImage so that when the failed percent still matches pendingPercent (and previewActive() is true), the loading state is cleared in the failure path before the finally block completes. Keep the fix localized to the preview fetch flow that updates img.src, pendingPercent, and lastFetchedPercent.frontend/src/views/files/Preview.vue (1)
321-323: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInclude
sourcein preview/transcode lifecycle keys.The current keying compares only
path; switching between sources with the same path can skip cleanup, reuse the wrongplyrViewerKey, and keep stale transcode state.Also applies to: 363-385
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/views/files/Preview.vue` around lines 321 - 323, The preview/transcode lifecycle keys are only based on state.req.path, so switching sources with the same path can reuse stale state; update plyrViewerKey and the related lifecycle key logic in Preview.vue to incorporate source as part of the key. Make sure every key used for preview/transcode cleanup and reinitialization distinguishes both path and source so a source change always forces the correct teardown and refresh.
♻️ Duplicate comments (2)
backend/http/transcode.go (1)
419-423: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winApply the
Realtimepermission guard to the release endpoint too.This transcode API can mutate active session state with only
TranscodeEnabled; keep it consistent with the other transcode handlers before releasing sessions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/http/transcode.go` around lines 419 - 423, The transcode release endpoint in transcodeSessionReleaseHandler currently only checks TranscodeEnabled, but it also mutates active session state and needs the same Realtime permission guard used by the other transcode handlers. Update transcodeSessionReleaseHandler to enforce the Realtime permission check before allowing a release operation, keeping its access control consistent with the related transcode endpoints.backend/http/stream_range.go (1)
157-160: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t send the full-file
Content-Lengthon the new 416 path.Line 158 calls
setStreamResponseHeaders(), which setsContent-Lengthto the full file size even though this branch returns a 416 error response.Proposed fix
if !ok { setStreamResponseHeaders(w, r, displayFileName, size) + w.Header().Del("Content-Length") w.Header().Set("Content-Range", fmt.Sprintf("bytes */%d", size)) return http.StatusRequestedRangeNotSatisfiable, fmt.Errorf("stream read ahead of playback window") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/http/stream_range.go` around lines 157 - 160, The 416 path in the stream range handler is incorrectly reusing setStreamResponseHeaders, which adds a full-file Content-Length even for an error response. Update the branch in stream_range.go around the !ok case in the stream response flow so it only sets the required range-related headers (for example via the existing Content-Range logic) and avoids calling setStreamResponseHeaders on this path. Keep the change scoped to the stream range response helpers and the handler that returns http.StatusRequestedRangeNotSatisfiable.
🧹 Nitpick comments (1)
backend/common/utils/stream_grant.go (1)
5-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the duplicated
StreamGrantmodel.
StreamGrantnow exists here and inbackend/http/stream_grant.go, so every payload change has to stay perfectly in sync across packages. This PR already had to touch both copies; using one shared type would avoid silent token/schema drift next time. As per path instructions, "reduce duplication where there is clear duplication found, use consolidated functions".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/common/utils/stream_grant.go` around lines 5 - 12, The StreamGrant payload is duplicated across packages, so changes must be made in two places and can drift silently. Consolidate the model by moving the shared StreamGrant definition into a single common package and updating both backend/common/utils/stream_grant.go and backend/http/stream_grant.go to reference that one type; use the existing StreamGrant symbol as the canonical schema for all token/payload handling.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/ffmpeg/hls.go`:
- Around line 51-58: BuildHLSSegmentOptions currently allows a negative index to
flow into the default StartSec calculation before the bounds checks, so invalid
segment requests can still produce ffmpeg options. Update the logic in
BuildHLSSegmentOptions to explicitly reject or short-circuit negative indexes
before computing StartSec or DurSec, keeping the existing starts/durations
overrides only for valid non-negative indexes. Use BuildHLSSegmentOptions and
HLSSegmentParams as the main reference points while making the rejection
behavior consistent with the rest of the segment validation.
In `@backend/http/stream_budget.go`:
- Around line 105-117: The forward-read limit is incorrectly anchored to the
original window, so the streaming range stops accepting contiguous reads after
the first span instead of sliding forward. Update the window-clamping logic in
the stream budget path that uses win.anchor, win.maxSpan, and win.highWater so
the allowed read-ahead advances with playback (for example, by basing the limit
on the current high-water/rolling window rather than a fixed anchor). Keep the
existing start/end bounds checks, but make sure the window can move forward as
data is consumed instead of permanently rejecting ranges once start reaches
windowEnd.
In `@backend/http/stream_range.go`:
- Around line 156-163: Cap the byte range earlier in stream_range.go so
open-ended requests don’t get treated as full-file reads before budgeting. In
the stream fetch flow around applyStreamFetchBudget and capStreamByteRange,
apply capStreamByteRange to the parsed start/end before calling
applyStreamFetchBudget, then keep the existing range rejection handling
afterward. Use the existing stream range helpers in stream_range.go to ensure
parseStreamByteRange, applyStreamFetchBudget, and capStreamByteRange work in the
intended order.
In `@backend/http/transcode_hls.go`:
- Around line 347-355: The playlist logging path is reading mutable HLS state
without synchronization, which can race with ensureHLSState updates and mix
stale metadata into responses. Add a read-snapshot helper on the HLS state
guarded by hls.mu, then update the transcode HLS flow to read params,
segmentCount, durationSec, and keyframeTimeline through that helper instead of
accessing entry.hls directly; apply the same pattern everywhere the comment
points to so hlsLogInfo, hlsUsesMPEGTS, and related response-building code use a
consistent snapshot.
- Around line 422-429: The retry path in HLS segment generation is mixing
full-transcode and video-copy modes within the same live session. In the HLS
segment handling flow around HLSInitAndSegment, do not just flip opts.VideoCopy
and reuse the existing timeline state; instead rebuild the HLS session/state for
the fallback mode before retrying, or return an error and skip the segment. Make
sure the fallback path also reinitializes any mode-specific settings tied to
Encode options so the segment is generated with a consistent timeline and
limits.
In `@backend/http/transcode_sessions.go`:
- Around line 267-289: The session tracking in transcodeSessionEntry currently
treats every reuse of the same userID+source+path as one stream, so distinct HLS
viewers in separate tabs/windows are not counted separately. Update the logic in
the session creation/reuse path and the release path in transcode_sessions.go so
that refreshes do not increment the viewer count, but each new viewer does, and
only tear down the shared HLS session when the last viewer releases it. Use
transcodeSessionEntry, s.sessions, and the release/cleanup logic around streams
to locate the code.
In `@backend/http/transcode.go`:
- Around line 424-432: The transcode handler is releasing sessions too broadly
in the missing-file path, because it ignores the requested source and calls the
user-wide release logic. Update the handling in transcode.go’s request flow
around the source and targetPath checks so that the cleanup is scoped to the
parsed source value, using a source-specific release method on
activeTranscodeSessions if available; if not, adjust the API contract to stop
requiring source. Keep the behavior in sync with the source validation in this
handler.
- Around line 52-59: The isH264VideoCodec helper is treating an empty codec
string as H.264, which can incorrectly send unknown metadata down the
stream-copy path; update this function to only return true for real H.264
aliases and not for empty input. Also add a unit test around the transcode/codec
detection logic to cover unknown or empty probe metadata and verify it falls
back to full transcode, using the isH264VideoCodec symbol to locate the
behavior.
In `@frontend/src/api/resources.js`:
- Around line 893-900: The releaseTranscodeSession(source, path) flow can
accidentally omit the file parameter and trigger a release-all on the backend
because getApiPath drops empty values. Update releaseTranscodeSession to require
a valid path for single-session release, and prevent calling it with undefined
or empty path; use the existing releaseTranscodeSession and getApiPath symbols
to ensure the DELETE request always includes file when releasing one session.
In `@frontend/src/utils/hlsTranscodePlayer.js`:
- Around line 21-27: The fragment metadata returned from hlsTranscodePlayer is
currently exposing full URLs, which can leak streamToken values into logs.
Update the helpers and event/logging paths in hlsTranscodePlayer so startup,
XHR, and fragment event messages use only non-sensitive metadata (for example
path, sn, type, status) or a redacted URL without query strings. Use the
existing frag-related helpers and the affected logging sites to ensure any url
field is sanitized before it is emitted.
- Around line 236-242: The startup promise in hlsTranscodePlayer.js resolves too
early in the MANIFEST_PARSED handler, before the watchdog can no longer fail
startup. Move the resolve logic in hls.on(Hls.Events.MANIFEST_PARSED) to the
first successful buffer path (for example the FRAG_BUFFERED/first-buffer
completion flow), or ensure the watchdog timeout is routed through onFatalError
even after resolve. Update the related startup handlers around the
resolve/rejectPromise/cleanup flow so playback only resolves once buffering has
actually begun and stalls still surface as fatal errors.
- Around line 78-93: The cleanup flow in hlsTranscodePlayer.js leaves videoEl
event listeners behind because the waiting and playing handlers are anonymous,
so remounts accumulate duplicate listeners and repeated encode-wait behavior.
Refactor the listener setup in the transcode player so the waiting/playing
handlers are stored as named references, then remove both listeners inside
cleanup alongside clearWatchdog(), signal?.removeEventListener('abort',
onAbort), and hls?.destroy(). Make the same listener teardown change anywhere
else the transcode player registers these handlers, including the other cleanup
path referenced by the duplicate comment.
In `@frontend/src/views/files/Preview.vue`:
- Around line 654-662: The teardown in releaseActiveTranscodeSession is
releasing every transcode session for the source instead of just the active one.
Use the tracked activeTranscodePath together with the source when calling the
API, and switch from releaseAllTranscodeSessions to releaseTranscodeSession in
Preview.vue’s releaseActiveTranscodeSession method so only the current session
is cleared.
- Around line 625-649: The async pre-start check in startVideoTranscode can act
on a different file if navigation changes state.req during the await. Capture
the current source and path before calling
resourcesApi.fetchTranscodeSessions(), then after the await verify they still
match state.req.source and state.req.path before setting
transcodePlaybackActive, activeTranscodeSource, activeTranscodePath, or calling
registerTranscodeSession; if they differ, abort and clear any pending offer.
---
Outside diff comments:
In `@backend/http/stream_grant_test.go`:
- Around line 15-29: Add a round-trip unit test for the new grant metadata by
updating one of the existing mint/validate paths in stream_grant_test.go to call
mintStreamGrant with non-zero FileSize and DurationSec values, then assert
validateStreamGrant accepts the token for the same requestContext and payload
shape. Use the existing mintStreamGrant and validateStreamGrant test flow so the
new transcode grant contract is exercised and regressions in the
FileSize/DurationSec payload fail fast.
In `@backend/preview/preview.go`:
- Around line 176-210: The preview cache key in GetPreviewForFile still omits
preserveAspectRatio, so square and non-square previews can collide for the same
file/size/seek values. Update the cache-key construction (the CacheKey call and
any matching key generation in related preview paths such as
GeneratePreviewWithMD5 and other referenced preview helpers) to include
preserveAspectRatio so each resize mode stores and reads distinct cached data.
In `@frontend/src/utils/plyrScrubPreview.js`:
- Around line 409-419: The scrubbing preview fetch in plyrScrubPreview.js leaves
loading stuck when the currently pending request fails without triggering an
image error. Update the try/catch around fetchPreviewImage so that when the
failed percent still matches pendingPercent (and previewActive() is true), the
loading state is cleared in the failure path before the finally block completes.
Keep the fix localized to the preview fetch flow that updates img.src,
pendingPercent, and lastFetchedPercent.
In `@frontend/src/views/files/Preview.vue`:
- Around line 321-323: The preview/transcode lifecycle keys are only based on
state.req.path, so switching sources with the same path can reuse stale state;
update plyrViewerKey and the related lifecycle key logic in Preview.vue to
incorporate source as part of the key. Make sure every key used for
preview/transcode cleanup and reinitialization distinguishes both path and
source so a source change always forces the correct teardown and refresh.
---
Duplicate comments:
In `@backend/http/stream_range.go`:
- Around line 157-160: The 416 path in the stream range handler is incorrectly
reusing setStreamResponseHeaders, which adds a full-file Content-Length even for
an error response. Update the branch in stream_range.go around the !ok case in
the stream response flow so it only sets the required range-related headers (for
example via the existing Content-Range logic) and avoids calling
setStreamResponseHeaders on this path. Keep the change scoped to the stream
range response helpers and the handler that returns
http.StatusRequestedRangeNotSatisfiable.
In `@backend/http/transcode.go`:
- Around line 419-423: The transcode release endpoint in
transcodeSessionReleaseHandler currently only checks TranscodeEnabled, but it
also mutates active session state and needs the same Realtime permission guard
used by the other transcode handlers. Update transcodeSessionReleaseHandler to
enforce the Realtime permission check before allowing a release operation,
keeping its access control consistent with the related transcode endpoints.
---
Nitpick comments:
In `@backend/common/utils/stream_grant.go`:
- Around line 5-12: The StreamGrant payload is duplicated across packages, so
changes must be made in two places and can drift silently. Consolidate the model
by moving the shared StreamGrant definition into a single common package and
updating both backend/common/utils/stream_grant.go and
backend/http/stream_grant.go to reference that one type; use the existing
StreamGrant symbol as the canonical schema for all token/payload handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4110917b-490e-4988-8287-5b52d57191f5
⛔ Files ignored due to path filters (30)
backend/go.sumis excluded by!**/*.sumbackend/swagger/docs/docs.gois excluded by!backend/swagger/**backend/swagger/docs/swagger.jsonis excluded by!backend/swagger/**backend/swagger/docs/swagger.yamlis excluded by!backend/swagger/**frontend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/src/i18n/ar.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/cz.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/de.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/el.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/en.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/es.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/fr.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/he.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/hu.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/it.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ja.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ko.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/nl-be.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/nl.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/pl.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/pt-br.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/pt.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ro.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ru.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/sk.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/sv-se.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/tr.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ua.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/zh-cn.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/zh-tw.jsonis excluded by!frontend/src/i18n/**
📒 Files selected for processing (30)
backend/common/utils/stream_grant.gobackend/ffmpeg/hls.gobackend/ffmpeg/hls_segments.gobackend/ffmpeg/hls_segments_test.gobackend/go.modbackend/http/httpRouter.gobackend/http/preview.gobackend/http/stream.gobackend/http/stream_budget.gobackend/http/stream_budget_test.gobackend/http/stream_grant.gobackend/http/stream_grant_test.gobackend/http/stream_range.gobackend/http/stream_range_test.gobackend/http/transcode.gobackend/http/transcode_hls.gobackend/http/transcode_hls_log.gobackend/http/transcode_hls_test.gobackend/http/transcode_sessions.gobackend/http/transcode_sessions_test.gobackend/http/transcode_test.gobackend/preview/preview.gofrontend/package.jsonfrontend/src/api/resources.jsfrontend/src/utils/hlsTranscodePlayer.jsfrontend/src/utils/plyrScrubPreview.jsfrontend/src/utils/plyrScrubPreview.test.jsfrontend/src/utils/transcodeSessionLifecycle.jsfrontend/src/views/files/Preview.vuefrontend/src/views/files/plyrViewer.vue
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/http/stream.go
- frontend/src/views/files/plyrViewer.vue
Co-authored-by: Graham <info@quantumx-apps.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/http/transcode_hls.go (2)
433-462: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftInclude
profilein HLS session identity.Line 433 keys the session only by user/source/path, but Line 462 initializes mutable HLS state for a requested profile. Two viewers of the same file with different profiles can reset each other’s cached params/playlist.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/http/transcode_hls.go` around lines 433 - 462, The HLS session is currently keyed only by user/source/path in activeTranscodeSessions.acquireHLS, but the mutable HLS state set up later by entry.ensureHLSState depends on the requested profile. Update the session identity used by acquireHLS/getHLSEntry to include the parsed profile from ffmpeg.ParseHLSTranscodeProfile so separate profile requests do not share the same entry and overwrite each other’s HLS state.
433-480: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease the acquired HLS stream when playlist setup fails.
After Line 433 increments/creates a session, probe/state/warmup errors return without decrementing
streams. A failed warmup can block the user until idle eviction.Suggested guard
acquire := activeTranscodeSessions.acquireHLS(d.user.ID, d.user.Username, fileReq.source, fileReq.scopedPath, fileReq.displayName) if !acquire.OK { switch acquire.Reason { @@ } } + keepSession := false + defer func() { + if !keepSession && acquire.Session != nil { + activeTranscodeSessions.releaseStream(acquire.Session.ID) + } + }() @@ if _, ok := entry.hls.cachedSegment(0); !ok { warmCtx, warmCancel := context.WithTimeout(r.Context(), hlsCfg.SegmentEncodeTimeout) _, _, warmErr := entry.encodeHLSSegment(warmCtx, svc, 0) @@ return http.StatusInternalServerError, fmt.Errorf("warm segment 0: %w", warmErr) } } entry.hls.encodeMu.Unlock() + keepSession = true
🧹 Nitpick comments (3)
frontend/src/utils/transcodePreference.js (1)
1-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the transcode-mode allowlist.
This duplicates the mode set already defined in
frontend/src/utils/playbackUrl.js, so future mode changes can drift between query parsing and localStorage validation. Reuse one shared constant/helper instead. As per path instructions, "reduce duplication where there is clear duplication found, use consolidated functions."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/utils/transcodePreference.js` around lines 1 - 8, The transcode-mode allowlist is duplicated between transcodePreference.js and playbackUrl.js, which can cause query parsing and localStorage validation to drift. Move the shared mode list into a single exported constant or helper in playbackUrl.js (or another shared utility) and have transcodePreference.js reuse it instead of defining VALID_PREFERRED_MODES locally. Update the validation logic around parseTranscodeModeFromQuery and the preferred-mode checks to reference the shared source of truth.Source: Path instructions
backend/http/transcode_sessions.go (1)
455-485: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDon’t evict the current segment when nothing is behind the playhead.
Line 478 picks the smallest
start - playhead; when all cached segments are current/future, that deletes the current segment first and keeps the furthest-ahead segments, causing avoidable re-encodes.Suggested adjustment
- // Evict segments furthest behind playhead first. + // Evict segments behind playhead first; otherwise evict the furthest ahead. for len(h.segments) > hlsMaxCachedSegments { worst := 0 for i := 1; i < len(items); i++ { - if items[i].dist < items[worst].dist { + if (items[i].dist < 0 && items[i].dist < items[worst].dist) || + (items[worst].dist >= 0 && items[i].dist > items[worst].dist) { worst = i } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/http/transcode_sessions.go` around lines 455 - 485, The pruneSegmentCache method is evicting the segment with the smallest start-playhead distance even when no cached segments are behind the playhead, which can remove the current segment first. Update hlsSessionState.pruneSegmentCache so the eviction choice prefers segments behind h.playheadSec only, and when all entries are current or ahead, preserve the current segment by skipping those candidates or falling back to the least harmful older entry. Keep the existing cache limit logic and adjust the selection in the items/worst loop accordingly.backend/ffmpeg/hls_config_test.go (1)
8-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the exported HLS header contract.
This suite exercises defaults and playlist metadata, but
WriteHLSConfigHeaders()is the new frontend-facing surface here and is still untested. A smallhttptest.ResponseRecordercase for the threeX-HLS-*headers would lock down that contract and catch drift with the playlist fallback.Example test shape
import ( + "net/http/httptest" "testing" "time" ) @@ func TestPlaylistConfigComment(t *testing.T) { comment := DefaultOnDemandHLSConfig().PlaylistConfigComment() want := "`#EXT-X-FB-CONFIG`:mode=on-demand;seg=4;buffer=3" if comment != want { t.Fatalf("comment = %q, want %q", comment, want) } } + +func TestWriteHLSConfigHeaders(t *testing.T) { + rr := httptest.NewRecorder() + WriteHLSConfigHeaders(rr, HLSConfig{}) + + if got := rr.Header().Get(hlsHeaderMode); got != string(HLSModeOnDemand) { + t.Fatalf("mode header = %q", got) + } + if got := rr.Header().Get(hlsHeaderSegmentDur); got != "4" { + t.Fatalf("segment duration header = %q", got) + } + if got := rr.Header().Get(hlsHeaderBufferSegments); got != "3" { + t.Fatalf("buffer segments header = %q", got) + } +}As per path instructions,
**/*.go: ensure functional features include new unit tests where its easily possible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/ffmpeg/hls_config_test.go` around lines 8 - 62, Add unit coverage for the exported header-writing contract in WriteHLSConfigHeaders, since the current tests only validate defaults and playlist metadata. Create a small httptest.ResponseRecorder-based test that calls WriteHLSConfigHeaders with a known HLSConfig and asserts the three X-HLS-* headers are set as expected. Use the existing HLSConfig, DefaultOnDemandHLSConfig, and WriteHLSConfigHeaders symbols to keep the test aligned with the frontend-facing API.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/ffmpeg/hls_config.go`:
- Around line 93-117: Normalize HLSConfig.Mode to the only supported backend
behavior instead of preserving arbitrary values; withDefaults() currently only
assigns HLSModeOnDemand when Mode is empty, which lets unsupported modes like
disk-cache or long-segment propagate through SetActiveHLSConfig() and then
surface in WriteHLSConfigHeaders() and PlaylistConfigComment(). Update
withDefaults() to coerce any non-empty Mode to the implemented default/only
valid mode, and ensure the header/comment generation paths continue to use the
normalized value from HLSConfig.
In `@backend/ffmpeg/hls_params.go`:
- Around line 10-18: Add unit coverage for the HLS transcode-decision helpers in
hls_params.go, since HLSNeedsFullVideoTranscode and HLSUseVideoCopy now
influence the request path in backend/http/transcode_hls.go. Create a small
table-driven test around the helper inputs (StreamInfo, HLSTranscodeProfile,
maxHeight) and assert the expected copy/remux/transcode selection for the key
cases, including the hlsPipelineOptions path used by both functions. Keep the
tests focused on these symbol names so regressions in decision logic are caught
early.
In `@backend/http/transcode_hls.go`:
- Around line 448-450: The lazy initialization of entry.hls can race across
concurrent playlist requests, so move creation of the HLS state into acquireHLS
while s.mu is held and make the entry.hls == nil check here an internal error
path instead of assigning it. Update the acquireHLS/session-store flow and the
transcode_hls state handling so hlsSessionState is always constructed under
synchronization and reused safely by the playlist handler.
In `@backend/http/transcode_session_ping.go`:
- Around line 27-30: In transcodeSessionPing, the JSON decode currently reads
directly from r.Body without any size limit, so wrap the request body with
http.MaxBytesReader before json.NewDecoder(r.Body).Decode(&body). Update the
transcodeSessionPing handler to bound the ping payload before decoding, keeping
the existing invalid ping body error path unchanged.
In `@frontend/src/utils/hlsTranscodePlayer.js`:
- Around line 405-417: Startup failures are being routed by playbackStarted,
which is set on MANIFEST_PARSED before the controller is actually resolved at
first buffer; this causes early fatal or fragment-loop errors to call
onFatalError instead of rejecting the startup Promise. Update failPlayback and
the surrounding startup flow in hlsTranscodePlayer.js to use controller
resolution state (for example, the point where FRAG_BUFFERED first confirms
startup) rather than manifest parsing to decide between reject and onFatalError.
Make sure the logic in the playback start path and the error handlers around the
referenced startup sequence keeps pending startup failures on the Promise until
the controller is truly ready.
- Around line 293-320: The native HLS branch in hlsTranscodePlayer.js returns a
controller with seekTo but never notifies the rest of the player about seek
events, unlike the hls.js path. Update the native HLS setup so the controller’s
seekTo logic also wires or emits the onSeek behavior used for session-cache
invalidation, and make sure the handler is attached alongside onReady/onError in
the native HLS branch. Use the existing seekTo, onReady, onError, and
addVideoListener flow to keep Safari/native HLS consistent with the hls.js
branch.
In `@frontend/src/utils/playbackUrl.js`:
- Around line 242-252: The playback query comparison in playbackQueryChanged is
using raw transcode values, which can treat equivalent modes as different and
trigger unnecessary resets. Update the comparison to normalize both
prev.transcode and next.transcode through parseTranscodeModeFromQuery (the same
normalization used elsewhere in playbackUrl.js) before comparing, while keeping
the existing rounded playback time check unchanged.
---
Outside diff comments:
In `@backend/http/transcode_hls.go`:
- Around line 433-462: The HLS session is currently keyed only by
user/source/path in activeTranscodeSessions.acquireHLS, but the mutable HLS
state set up later by entry.ensureHLSState depends on the requested profile.
Update the session identity used by acquireHLS/getHLSEntry to include the parsed
profile from ffmpeg.ParseHLSTranscodeProfile so separate profile requests do not
share the same entry and overwrite each other’s HLS state.
---
Nitpick comments:
In `@backend/ffmpeg/hls_config_test.go`:
- Around line 8-62: Add unit coverage for the exported header-writing contract
in WriteHLSConfigHeaders, since the current tests only validate defaults and
playlist metadata. Create a small httptest.ResponseRecorder-based test that
calls WriteHLSConfigHeaders with a known HLSConfig and asserts the three X-HLS-*
headers are set as expected. Use the existing HLSConfig,
DefaultOnDemandHLSConfig, and WriteHLSConfigHeaders symbols to keep the test
aligned with the frontend-facing API.
In `@backend/http/transcode_sessions.go`:
- Around line 455-485: The pruneSegmentCache method is evicting the segment with
the smallest start-playhead distance even when no cached segments are behind the
playhead, which can remove the current segment first. Update
hlsSessionState.pruneSegmentCache so the eviction choice prefers segments behind
h.playheadSec only, and when all entries are current or ahead, preserve the
current segment by skipping those candidates or falling back to the least
harmful older entry. Keep the existing cache limit logic and adjust the
selection in the items/worst loop accordingly.
In `@frontend/src/utils/transcodePreference.js`:
- Around line 1-8: The transcode-mode allowlist is duplicated between
transcodePreference.js and playbackUrl.js, which can cause query parsing and
localStorage validation to drift. Move the shared mode list into a single
exported constant or helper in playbackUrl.js (or another shared utility) and
have transcodePreference.js reuse it instead of defining VALID_PREFERRED_MODES
locally. Update the validation logic around parseTranscodeModeFromQuery and the
preferred-mode checks to reference the shared source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a27b23c2-8464-48f9-89f4-afcf59bdab44
⛔ Files ignored due to path filters (29)
backend/go.sumis excluded by!**/*.sumbackend/swagger/docs/docs.gois excluded by!backend/swagger/**backend/swagger/docs/swagger.jsonis excluded by!backend/swagger/**backend/swagger/docs/swagger.yamlis excluded by!backend/swagger/**frontend/src/i18n/ar.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/cz.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/de.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/el.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/en.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/es.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/fr.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/he.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/hu.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/it.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ja.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ko.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/nl-be.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/nl.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/pl.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/pt-br.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/pt.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ro.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ru.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/sk.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/sv-se.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/tr.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/ua.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/zh-cn.jsonis excluded by!frontend/src/i18n/**frontend/src/i18n/zh-tw.jsonis excluded by!frontend/src/i18n/**
📒 Files selected for processing (39)
CHANGELOG.md_docker/Dockerfilebackend/common/settings/config.gobackend/ffmpeg/hls.gobackend/ffmpeg/hls_config.gobackend/ffmpeg/hls_config_test.gobackend/ffmpeg/hls_describe.gobackend/ffmpeg/hls_encode.gobackend/ffmpeg/hls_encode_test.gobackend/ffmpeg/hls_params.gobackend/ffmpeg/hls_segments_test.gobackend/ffmpeg/service.gobackend/go.modbackend/http/httpRouter.gobackend/http/stream_budget.gobackend/http/stream_budget_test.gobackend/http/stream_range.gobackend/http/transcode.gobackend/http/transcode_errors.gobackend/http/transcode_errors_test.gobackend/http/transcode_hls.gobackend/http/transcode_hls_log.gobackend/http/transcode_hls_test.gobackend/http/transcode_session_ping.gobackend/http/transcode_sessions.gobackend/http/transcode_test.gofrontend/src/api/resources.jsfrontend/src/utils/hlsTranscodeConfig.jsfrontend/src/utils/hlsTranscodePlayer.jsfrontend/src/utils/playbackUrl.jsfrontend/src/utils/playbackUrl.test.jsfrontend/src/utils/plyrScrubPreview.jsfrontend/src/utils/plyrScrubPreview.test.jsfrontend/src/utils/transcodePreference.jsfrontend/src/utils/transcodePreference.test.jsfrontend/src/utils/transcodeSessionLifecycle.jsfrontend/src/views/Files.vuefrontend/src/views/files/Preview.vuefrontend/src/views/files/plyrViewer.vue
✅ Files skipped from review due to trivial changes (2)
- frontend/src/utils/playbackUrl.test.js
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (11)
- backend/http/transcode_hls_log.go
- backend/http/transcode_hls_test.go
- backend/http/stream_budget.go
- backend/common/settings/config.go
- backend/http/httpRouter.go
- backend/http/stream_budget_test.go
- frontend/src/utils/plyrScrubPreview.test.js
- backend/http/stream_range.go
- frontend/src/views/files/Preview.vue
- frontend/src/utils/plyrScrubPreview.js
- frontend/src/views/files/plyrViewer.vue
Collaborator
|
Just wow, I'm without words... Man, seriously, is awesome!! Not sure if is fully finished, but I just want to leave a bit of feedback from what I tested until now :)
|
Owner
Author
|
yeah this one has taken a lot more work than I thought it would. I thought transcoding would be simple, theres way more complexity needed than I expected. For now, I think its best to keep the transcoding options to a minimum. The way it works now is a zero disk-cache on-demand transcoding. Which means the ffmpeg reads the file and at certain segments and sends the output directly to the api without writing to disk. This has the advantage of being less cache-intensive and potentially better performance. But I am not sure this will be the longterm stategy. I think I may add a minimal 30 minute disk cache retention option. It really just depends on how how users use it... if its just quick look at a random video, a zero cache implmentation works very well. But if certain videos are commonly watched by many users at certain users, it might be worth adding a bit of cache. Regarding the quality vs data saver options. "quality" is vague on purpose, because its not always the same behavior. if the video transcodes, then it has an upper limit of 1080p. But quality doesn't always transcode the video. For example, if a browser has an mkv mp4 container but uses a non standard audio like E-AC3 , it only transcodes the audio and copies the video. So I didn't want to create a hard rule where it does "x", its just a "make it play" kinda mode. Data saver does always transcode video, and has a max resolution of 720p with an aggressive bitrate max.
yeah this is something that needs adjustment. The backend must send certain keyframes. 1 minute seems excessive. it should be only a few seconds difference at most. I will look into that. But because of the keyframes, a scrub may be off by a few seconds -- although it needs to be adjusted to always prefer going to the nearest keyframe to the left (older) not forward.
this is going to be a limitation. its hardware. there could be a throttle that debounces this behavior. But hammering the hardware with several requests will cause issues. This is going to be a limitation of transcoding in general, you can overload the hardware very easily. And software encoding is super dangerous because it can lock up the CPU without much effort... so for now I plan to allow it but will probably put a giant warning that it is not reliable at any scale beyond 1 or 2 users.
This is a good question, its based on your application settings. like in windows you can set an application to use the dedicated gpu. on linux, not sure how to handle it, haven't really gotten that far into it. and may not for the 2.0.0 release... its not going to be perfect.
will look at this, you mean like playlist prompts? |
Collaborator
|
Maybe make the cache configurable (enable/disable/retention time...)? I think some kind of smart mode to track if a video is viewed or not would be more complex to do, and may or not work as expected (idk honestly). I wonder how other software do that (and if they do it), because if the cache comes hard-coded, some might want to make it last long, or short because is wasting space, or disable it because they don't want to cause wear to their disks. I'm not sure how all works behind the scenes (so there's not much I can do more than give some ideas) :,) About the transcoding options, I remember saw a "balanced" option in the code (never saw it on UI tho). With those 3 options I think shouldn't be any issue.
And btw, when tried the "data saver" option was strange cuz you said it has a max res of 720p, but the video displayed was very low quality like if were 240p or even 144p. (the video originally was 720p), was very blurry.
yup, but I was referring mostly to the UI, the timestamp/number displayed when hovering the mouse, I don't care much if the previews are a bit off. On short videos like 2 minutes or so seems to be fine, but is worse when the video is very long. The one of plyr is not accurate either but is more precise.
No issues btw, is really good anyways! I use Linux with a Window Manager (hyprland) in Wayland. In my case seems to be choosing the GPU that I use in my WM, if I use the dedicated one, it chooses that. Not sure how is on another DE/WM or in headless servers though. edit: on docker at least, we can choose which GPU to use, we should document this when the version releases!
Whatever prompt, but the more affected is the playback queue since we can play/pause doing click in the current item. You can open the info prompt with the 3 dots at the top and press |
This comment was marked as off-topic.
This comment was marked as off-topic.
Owner
Author
|
I had cursor take some inspiration from the jellyfin repo to see how it was done there. Jellyfin always caches so having a no-cache solution couldn't be replicated exactly. youre right thats a good configuration to add |
Owner
Author
|
for now I will add: but in a different pr much of this I will remove from the config and make a database property that gets updated by admin in settings > media. (The same will come for user defaults, to be removed from the config file) |
Collaborator
|
that's even better! |
Owner
Author
|
im going to close this. theres a ton of issues, this is going to be a support nightmare if not done correctly. So it will probably need to wait since its blocking other things and taking up my time |
Collaborator
|
seems fair, take your time with this. I wish I could do something to help, but is beyond from what I know :,) |
Owner
Author
|
I learned a lot while testing, I will make a release dedicated just to it. I don't want to implement something thats going to have a lot of bugs. Maybe v2.1.0 or something will introduce transcoding. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
According to the contributing guide, A PR should contain:
Additional Details
Summary by CodeRabbit
New Features
Bug Fixes