Honor marker end times; make the ceiling configurable#6855
Open
speckofthecosmos wants to merge 2 commits intostashapp:developfrom
Open
Honor marker end times; make the ceiling configurable#6855speckofthecosmos wants to merge 2 commits intostashapp:developfrom
speckofthecosmos wants to merge 2 commits intostashapp:developfrom
Conversation
Replaces the hardcoded maxMarkerPreviewDuration = 20 constant in pkg/scene/generate/marker_preview.go with a configurable setting on ConfigGeneral, mirroring the PreviewSegments wiring pattern. - Default 20 preserves existing behavior for all installs - Positive N sets the ceiling in seconds - 0 (or any value <= 0) disables the ceiling, honoring explicit endSeconds verbatim - Adds logger.Warnf for non-positive intervals to aid diagnosis of imported or malformed marker data - Incidentally prevents zero/negative-duration ffmpeg calls that were possible under the previous logic Context: closed issue stashapp#6852 (template non-conformance), Discussion stashapp#6851 (mobile-first direction). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to the initial ceiling-configurable change, incorporating product-review feedback. Two design shifts plus a read-side bug fix: - Default MaxMarkerPreviewDuration flipped from 20 to 0 (ceiling disabled). User-set end times are now honored by default; the ceiling becomes an opt-in safety for imports or untrusted data. A buried setting at its old 20s default was unlikely to be found by users hitting the silent-truncation case. - Non-positive intervals (endSeconds <= seconds) now skip preview generation entirely rather than falling back to the 20s default. A user who sets zero or negative duration either made a mistake (don't invent output) or marked a point without wanting a video (don't generate one they didn't ask for). Either way, skip with a warning log. - Fix missing ConfigGeneralResult field wiring in resolver_query_configuration.go. The schema and mutation sides were wired in the initial commit but the query read path was missed, causing the Settings UI to display Go's zero-value (0) regardless of the stored config value. Behavior change on upgrade: existing preview files remain unchanged. To refresh previews for markers whose end times were previously truncated at 20s, run Generate > Marker Previews with "Overwrite Existing" enabled. To keep the old 20s ceiling, set Max marker preview duration to 20 under Settings > System > Preview Generation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Author
|
For anyone wanting to test this before merge: https://github.com/speckofthecosmos/stash-marker-cap-patch Public multi-arch image at Verified working on Synology NAS + verified end-to-end: 41 affected markers (with end_seconds beyond 20s) regenerated with correct durations. |
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
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.
Scene markers gained explicit end times in PR #5311 (November 2024), but
pkg/scene/generate/marker_preview.gowas not updated to consume them. The file'smaxMarkerPreviewDuration = 20constant predates end times by two years (PR #2392, April 2022). For the 18 months since #5311 shipped, user-set end times beyond 20 seconds have been silently truncated at preview generation: the UI accepts the value, the database stores it, the marker form displays it, but the generated video is capped. The end-time feature has been shipping incomplete.This PR closes the loop. Preview generation honors the marker's
endSecondsby default. The 20s ceiling becomes opt-in via a newMaxMarkerPreviewDurationsetting for users who want it as a safety against imports or data entry mistakes.The cost of delay is growing. Every marker created since November 2024 with
endSeconds > 20is a marker whose preview file doesn't match its stored metadata. That misalignment compounds daily — users shipping the fix later will have more stale preview files to regenerate than users shipping it now.Change
New
Max marker preview durationsetting under Settings → System → Preview Generation.endSeconds <= seconds) — preview generation is skipped with a warning log, rather than falling back to a default. If the user set zero or negative duration, either it's a data mistake (surface it, don't silently invent output) or it's an intentional point marker (respect the signal, don't generate a video they didn't ask for).endSecondscontinue to use a fixed 20s default (deliberately out of scope for this PR — see below).Why the default is 0
A buried opt-in setting is the wrong interaction pattern for user-created data. Most users who set a marker end time expect it to be honored; the silent-truncation case penalized the majority for an edge case (imports with unusually long end times). With the flipped default, the ceiling-as-safety remains available via one setting for users who want it, and the common path matches user intent.
The 0-disables convention follows established precedent in
config.go—defaultLogFileMaxSize = 0 // megabytes, default disabled.Incidental fix
The new
interval > 0guard prevents zero/negative-interval markers from producing malformed ffmpeg calls — previously possible ifendSeconds <= seconds, producing zero or negative duration arguments.Behavior change on upgrade
Existing marker preview files have had silently-truncated durations since #5311 shipped 18 months ago. This PR does not change those files retroactively — they remain until regenerated.
To refresh previews for markers whose end times have never been honored, run Generate → Marker Previews with Overwrite Existing enabled. Note: this regenerates ALL marker previews, not just the affected subset. For large libraries with few affected markers, this is imperfect — a targeted "refresh stale previews" task would be a natural follow-up enhancement and is out of scope here.
To keep the old 20s ceiling, set Max marker preview duration to 20 under Settings → System → Preview Generation.
Closes/Fixes Issues
Testing QA Notes
endSeconds=40s, seconds=10srenders full 30s interval ✓endSeconds: uses 20s default regardless of cap setting ✓endSeconds <= seconds: generation skipped, warning log recorded ✓make generate); backend and frontend both compile cleanlyOut of scope
Making the 20s default (for markers without explicit
endSeconds) configurable. Kept separate because the cap change fixes a demonstrable bug (silent truncation of user-provided end times), while making the nil-default configurable is a preference surface that invites distinct design discussion — what should it default to? Same as the cap? A 5–10s teaser? 0 meaning "skip generation for bookmark-style markers"?Happy to bundle into this PR if reviewers think UI-coherence (changing Settings → System once rather than twice) outweighs narrow-scope. The incremental work is small:
Plus mirror plumbing:
setConfigIntin the resolver, field in GraphQLConfigGeneralInput/Result, a second<NumberSetting>inSettingsSystemPanel.tsx, i18n strings for head/desc.Otherwise, a follow-up PR would cite Discussion #6851 for framing and propose a sharper user story around the nil-default.
Also worth a follow-up PR: a "Refresh Outdated Marker Previews" maintenance task that detects markers whose existing preview duration doesn't match the expected duration from current config, and regenerates only those. Useful for this PR's migration story, useful anytime config changes affect preview output, and useful independently of this PR.