fix: derive timelapse duration from compiled video#192
Conversation
The `duration` field was computed from snapshot timestamps using a 120-second idle timeout filter, which often diverged from the actual video length. This caused ratios of 75x-162x instead of the expected 60x between `duration` and video length. Now when the realize worker finishes compiling a video, it returns the measured video duration (already computed for thumbnail generation) and the server overwrites `duration` with `videoDuration * TIMELAPSE_FACTOR`. Also guards `recalculateDurations` to only target unrealized timelapses, preserving video-derived durations for completed ones.
There was a problem hiding this comment.
Pull request overview
Adjusts how published timelapse duration is derived so API-reported real-time duration matches the compiled video length (instead of relying on snapshot timestamp heuristics), and prevents the admin duration recalculation endpoint from overwriting video-derived durations.
Changes:
- Extend realize job outputs to include optional
videoDuration(seconds) measured viaffprobe. - Have the worker return
videoDuration, and have the server persistduration = videoDuration × TIMELAPSE_FACTORon job completion when present. - Restrict the
recalculateDurationsadmin endpoint to only update unrealized timelapses (s3Key: null).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/jobs/src/realize.ts | Adds videoDuration to the realize job output schema for server-side persistence. |
| apps/worker/src/workers/realize.ts | Captures and returns videoDuration alongside the video/thumbnail S3 keys. |
| apps/server/src/routers/admin.ts | Limits duration recalculation to unrealized timelapses and updates log messaging. |
| apps/server/src/job.ts | Persists video-derived duration on realize completion and logs videoDuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/jobs/src/realize.ts
Outdated
| * The duration of the compiled output video in seconds, as measured by ffprobe. | ||
| * Optional for backwards compatibility with in-flight jobs that predate this field. | ||
| */ | ||
| videoDuration: z.number().nonnegative().optional() |
There was a problem hiding this comment.
videoDuration is validated as nonnegative(), which allows 0, but measureVideoDuration() throws for duration <= 0. Consider using positive() (or finite().positive()) here so the schema matches the actual invariant and rejects zero/NaN values.
| videoDuration: z.number().nonnegative().optional() | |
| videoDuration: z.number().finite().positive().optional() |
| // Only recalculate durations for timelapses that haven't been realized yet. | ||
| // Realized timelapses have their duration set from the compiled video, which is the source of truth. | ||
| const PAGE_SIZE = 100; | ||
| let updated = 0; | ||
| let cursor: string | undefined; | ||
|
|
||
| while (true) { | ||
| const batch = await database().timelapse.findMany({ | ||
| select: { id: true, snapshots: true }, | ||
| where: { s3Key: null }, |
There was a problem hiding this comment.
This endpoint now explicitly skips realized timelapses (where: { s3Key: null }), but the public admin contract/docs still describe it as “recalculates the duration of every timelapse from its snapshots”. Please update the API contract description (and/or rename/add params) so callers aren’t misled about what will be updated.
| data: { | ||
| associatedJobId: null, | ||
| s3Key: videoKey, | ||
| thumbnailS3Key: thumbnailKey | ||
| thumbnailS3Key: thumbnailKey, | ||
| ...(videoDuration != null && { duration: videoDuration * TIMELAPSE_FACTOR }) | ||
| }, |
There was a problem hiding this comment.
Duration is derived using a runtime TIMELAPSE_FACTOR import. Since the server and worker ship as separate images, a version-skew deploy could result in the worker encoding with one factor while the server multiplies by another, permanently storing the wrong duration. To make this robust, consider having the worker return the factor used (or the already-multiplied real-time duration) and use that value for the DB update.
The `duration` field was computed from snapshot timestamps using a 120-second idle timeout filter, which often diverged from the actual video length. This caused ratios of 75x-162x instead of the expected 60x between `duration` and video length. Now when the realize worker finishes compiling a video, it returns the real-time duration (videoDuration * TIMELAPSE_FACTOR, computed worker- side to avoid version-skew issues) and the server stores it directly. Also guards `recalculateDurations` to only target unrealized timelapses, preserving video-derived durations for completed ones.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/jobs/src/realize.ts
Outdated
| * The real-time duration of the timelapse in seconds (i.e. `videoDuration * TIMELAPSE_FACTOR`), | ||
| * as measured by ffprobe on the compiled output video. This value can be stored directly as the | ||
| * timelapse `duration` without further conversion. |
There was a problem hiding this comment.
The JSDoc for realTimeDuration is a bit contradictory: ffprobe measures the compiled video’s duration (videoDuration), but realTimeDuration is a derived value after multiplying by TIMELAPSE_FACTOR. Consider updating the comment to explicitly say it’s derived from the ffprobe-measured duration (or alternatively rename/expose the raw videoDuration separately) to avoid confusing future consumers about units/source-of-truth.
| * The real-time duration of the timelapse in seconds (i.e. `videoDuration * TIMELAPSE_FACTOR`), | |
| * as measured by ffprobe on the compiled output video. This value can be stored directly as the | |
| * timelapse `duration` without further conversion. | |
| * The real-time duration of the timelapse in seconds, derived as | |
| * `videoDuration * TIMELAPSE_FACTOR` from the compiled output video's duration measured by ffprobe. | |
| * This value can be stored directly as the timelapse `duration` without further conversion. |
| return { | ||
| timelapseId, | ||
| videoKey, | ||
| thumbnailKey | ||
| thumbnailKey, | ||
| realTimeDuration: videoDuration * TIMELAPSE_FACTOR | ||
| }; |
There was a problem hiding this comment.
realTimeDuration is being returned as videoDuration * TIMELAPSE_FACTOR (i.e., already converted to the timelapse’s “real-time” duration). Given the PR description talks about returning the measured videoDuration and letting the server multiply by 60, it would be clearer to either (a) return the raw videoDuration and do the conversion server-side, or (b) rename this field to something that makes it obvious it’s already in timelapse-duration seconds (and optionally log/return both values).
packages/api/src/contracts/admin.ts
Outdated
|
|
||
| recalculateDurations: contract("POST", "/admin/recalculateDurations") | ||
| .route({ description: "Recalculates the duration of every timelapse from its snapshots. Requires administrator permissions and an `elevated` grant." }) | ||
| .route({ description: "Recalculates the duration of unrealized timelapses (those still processing, without a compiled video) from their snapshots. Realized timelapses are skipped because their duration is derived from the compiled video. Requires administrator permissions and an `elevated` grant." }) |
There was a problem hiding this comment.
The updated route description says unrealized timelapses are “those still processing”, but the server-side filter is where: { s3Key: null }, which will also include timelapses that are no longer processing (e.g. FAILED_PROCESSING with associatedJobId: null). Consider tweaking the description to match the actual criterion (“no compiled video / s3Key is null”) rather than “still processing”.
| .route({ description: "Recalculates the duration of unrealized timelapses (those still processing, without a compiled video) from their snapshots. Realized timelapses are skipped because their duration is derived from the compiled video. Requires administrator permissions and an `elevated` grant." }) | |
| .route({ description: "Recalculates the duration of unrealized timelapses (those without a compiled video, where `s3Key` is null) from their snapshots. Realized timelapses are skipped because their duration is derived from the compiled video. Requires administrator permissions and an `elevated` grant." }) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
thank you! |
Summary
durationfield was computed from snapshot timestamps with a 120s idle timeout filter, which often diverged from the actual video length (ratios of 75x–162x instead of expected 60x)durationwithvideoDuration × 60recalculateDurationsadmin endpoint to only target unrealized timelapses, preserving video-derived durationsTest plan
duration / 60 ≈ video_lengthin the API responsevideoDurationin the job completion outputrecalculateDurationsonly updates timelapses withs3Key: nullvideoDuration) still complete without error