fix: CasparCG clip duration calculation for frame-rate specific formats#2
Conversation
- Implement comprehensive framerate parsing for CasparCG 2.5 compatibility - Handles framerate formats: direct fps, fps1000, and fps1001 - Correctly detects fractional rates (29.97fps, 59.94fps) vs integer rates (30fps, 60fps) - Fixes duration calculation for 1080i5994 clips - Add frameTime calculation (timecode format HH:MM:SS:FF) - Improve error handling and logging with device ID context - Add duration validation in resources.ts to prevent NaN/Infinity propagation - Add debug logging for problematic clips - 1080i5994, high framerates Fixes frame-rate specific duration parsing issue
…G_clip_duration_fixes Test bugfix caspar cg clip duration fixes
fix(tsr-bridge): use explicit .js ESM import for helpers ci: use upstream Node CI workflow (node.yaml) chore(tsr-bridge): fix lint/prettier issues; include tests in package tsconfigs; remove unused import chore: remove non-PR files and revert temp test script; keep only casparcg fix + helpers + tests tests: add .js extensions & include jest types in tsconfigs to fix typecheck tests: fix type-check (add input mock for OBS input types) lint: fix Prettier reflows and add ESLint exceptions for fetch usage; fix test indentation lint: fix Prettier indentation and helper signatures; align telemetry ESLint comment fix: correct logic/formatting in CasparCG and telemetry; Prettier conformances style(tsr-bridge): align indentation for Prettier fix(tsr-bridge): restore resource object (lost during formatting) Fix Prettier indentation in CasparCG and telemetry Restore missing resource fields (id, type, name) in CasparCG resource
…rce locator, formatDurationLabeled)
ci: refresh workflows (push refresh) style: fix Prettier whitespace in timeLib.ts ci: run electron-builder per workspace to fix macOS packaging ci: set GH_TOKEN to GITHUB_TOKEN for binary builds ci: add GH_TOKEN & isolated electron-builder cache, retries, and defensive artifact moves ci(windows): run build:binary under bash so retry helper works ci(macos): make notarize defensive and add debug logs for missing app path ci(lint): allow require() for fs in notarize.cjs to satisfy linter ci: add prep-build-binary wrapper to log expected paths before electron-builder (linted) ci: use CommonJS prep-build-binary wrapper for tsr-bridge chore: remove ESM prep-build-binary script fix(tsr-bridge): prep-build-binary lint/prettier (remove shebang, format tabs) ci(macos): add pre-build diagnostics, clear builder cache, upload builder configs
scripts(tsr-bridge): make prep-build-binary robust to missing npx and avoid throwing scripts(tsr-bridge): robust shell fallback for electron-builder (npx→yarn→local) Update packageManager to yarn@4.12.0
…onsole, catch binding) fix(tsr-bridge): clean electron-output before binary build and ensure electron-builder availability ci: set XDG_CACHE_HOME early in macOS Prepare Environment step fix(tsr-bridge): use yarn exec for electron-builder in Yarn workspaces
fix(ci): correct secrets conditional syntax in workflow fix(ci): use proper secrets syntax in if condition fix(ci): use environment variable for secrets check in if condition
There was a problem hiding this comment.
Pull request overview
This pull request fixes CasparCG clip duration calculation issues for frame-rate specific formats, particularly addressing problems with 1080i5994 clips and other fractional framerates.
Key changes include:
- Comprehensive framerate parsing logic that handles direct fps values, fps1000, and fps1001 formats with proper detection of fractional rates (29.97fps, 59.94fps) versus integer rates
- Addition of frameTime calculation in HH:MM:SS:FF timecode format for CasparCG media resources
- Enhanced error handling and logging with device ID context throughout the CasparCG sideload process
- Duration validation in resources.ts to prevent NaN/Infinity values from propagating to timeline objects
- UI improvements to gracefully handle missing or invalid duration values in the resource library
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| shared/packages/tsr-bridge/src/sideload/CasparCG.ts | Implements comprehensive framerate parsing and duration calculation logic with improved error handling and logging |
| apps/app/src/react/components/sidebar/resource/ResourceLibraryItem.tsx | Adds conditional rendering to display duration only when valid |
| apps/app/src/lib/timeLib.ts | Extends formatDurationLabeled to display milliseconds for sub-second durations |
| apps/app/src/lib/resources.ts | Adds validation to ensure only valid, finite duration values are used in timeline objects |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else { | ||
| if (fpsBy1000 >= 20 && fpsBy1000 <= 70) { | ||
| framerateFps = fpsBy1000 | ||
| } else if (fpsBy1001 >= 20 && fpsBy1001 <= 70) { | ||
| framerateFps = fpsBy1001 | ||
| } else { | ||
| framerateFps = fpsBy1000 | ||
| } | ||
| } |
There was a problem hiding this comment.
The framerate detection logic has overlapping conditions. Lines 155-162 check both fpsBy1000 and fpsBy1001 against the same 20-70 range, but the else branch at line 160 defaults to fpsBy1000 regardless. This means if both values are outside the 20-70 range, the code arbitrarily chooses fpsBy1000 without proper justification. Consider adding validation or logging to handle edge cases where neither value falls within expected ranges.
| if (media.framerate > 1000 || duration < 0.1 || duration > 3600 || media.clip.includes('5994')) { | ||
| this.log.info( | ||
| `Clip "${media.clip}": frames=${media.frames}, raw_framerate=${media.framerate}, calculated_fps=${framerateFps.toFixed(2)}, duration=${duration.toFixed(2)}s` | ||
| ) | ||
| } |
There was a problem hiding this comment.
The debug logging condition at line 167 includes media.clip.includes('5994') which is checking for a specific string pattern in the filename rather than a property of the media format. This is a fragile approach that assumes filename conventions. Consider logging based on actual format properties (like framerate values) rather than filename patterns, or document why this specific pattern is being checked.
| duration = (media as any).duration | ||
| if (media.framerate != null && media.framerate > 0) { | ||
| framerateFps = media.framerate > 1000 ? media.framerate / 1000 : media.framerate | ||
| } |
There was a problem hiding this comment.
At line 130, when calculating framerateFps from an already-provided duration, the code doesn't validate that framerateFps is used anywhere after this point in this branch. The variable is set but since duration is already calculated from (media as any).duration, the framerateFps value is only used later for frameTime calculation (line 192). Consider clarifying the purpose of this calculation or removing it if it's not needed in this branch.
| } else if (ms > 0 && !h && !m) { | ||
| returnStr += `${ms}ms` |
There was a problem hiding this comment.
The new millisecond formatting logic for durations less than 1 second is not covered by tests. Consider adding test cases in timeLib.test.ts to verify that formatDurationLabeled correctly formats durations like 500ms, 100ms, etc., especially since the existing formatDuration function has extensive test coverage.
| let durationMs: number | undefined = undefined | ||
| if ( | ||
| resource.duration != null && | ||
| typeof resource.duration === 'number' && | ||
| isFinite(resource.duration) && | ||
| resource.duration >= 0 | ||
| ) { | ||
| durationMs = resource.duration * 1000 | ||
| } |
There was a problem hiding this comment.
The new duration validation logic for CasparCG media resources should have test coverage. Consider adding tests in resources.test.ts to verify that invalid duration values (NaN, Infinity, negative values) are properly handled and that only valid durations are used in the timeline object generation.
| * Calculate duration safely, handling edge cases for CasparCG 2.5 compatibility | ||
| * CasparCG 2.5 may store framerate in different formats: | ||
| * As fps directly (e.g., 30 for 30fps) | ||
| * As fps * 1000 (e.g., 30000 for 30fps) - most common in CasparCG 2.5 | ||
| * First, check if duration is provided directly (preferred) |
There was a problem hiding this comment.
The comment at line 116 mentions "fps * 1001" but this format is not actually handled in the code logic. The code only handles division by 1000 and 1001, not multiplication. If CasparCG 2.5 can store framerates as "fps * 1001" (e.g., 29970 for 29.97fps), this case is not properly handled. Consider clarifying the comment or adding logic to handle this format.
| * Calculate duration safely, handling edge cases for CasparCG 2.5 compatibility | |
| * CasparCG 2.5 may store framerate in different formats: | |
| * As fps directly (e.g., 30 for 30fps) | |
| * As fps * 1000 (e.g., 30000 for 30fps) - most common in CasparCG 2.5 | |
| * First, check if duration is provided directly (preferred) | |
| * Calculate duration safely, handling edge cases for CasparCG 2.5 compatibility. | |
| * | |
| * Currently supported framerate formats: | |
| * - As fps directly (e.g., 30 for 30fps) | |
| * - As fps * 1000 (e.g., 30000 for 30fps), which is common in CasparCG 2.5 | |
| * | |
| * Note: Other encodings (for example, fps * 1001) are not interpreted here and | |
| * would require additional parsing logic if they occur in practice. | |
| * First, check if duration is provided directly (preferred). |
| } else if (ms > 0 && !h && !m) { | ||
| returnStr += `${ms}ms` |
There was a problem hiding this comment.
The condition at line 238 checks ms > 0 && !h && !m but this is already partially handled by the s check at lines 232-237. The new else-if will only execute when seconds are also 0. However, line 238 checks ms > 0 while the containing condition at line 232 checks if (s), meaning when s is 0 but ms > 0, this would display milliseconds. This appears to be the intended behavior but could benefit from a comment explaining this handles the edge case of durations less than 1 second.
| const fps = Math.round(framerateFps) // Round to integer for timecode | ||
|
|
||
| const frames = totalFrames % fps |
There was a problem hiding this comment.
The frameTime calculation at line 196 computes frames = totalFrames % fps, but this uses the rounded fps value. For non-drop-frame timecode with fractional framerates like 29.97, using a rounded value (30) will cause the frame count to be incorrect. Consider whether this should use the actual fractional framerate or if the rounding is intentional for timecode display purposes. If intentional, add a comment explaining this is for timecode compatibility.
| } else if (fpsBy1000 >= 20 && fpsBy1000 <= 70) { | ||
| framerateFps = fpsBy1000 |
There was a problem hiding this comment.
The logic in the if-else branches at lines 149-150 and 151-152 is redundant. Both branches execute framerateFps = fpsBy1000, making the conditional check unnecessary. This suggests the logic may not be correctly handling all cases for fps1000 vs fps1001 detection when the framerate is divisible by 1000.
| } else if (fpsBy1000 >= 20 && fpsBy1000 <= 70) { | |
| framerateFps = fpsBy1000 |
- Add autoStep property to Group model for cycling through parts at scheduled times - Implement Auto Step logic in scheduler to cycle through parts sequentially - Add Auto Step UI control in Group Settings sidebar (visible when in Schedule mode) - Add Auto Step toggle button in Group View header (visible when in Schedule mode) - Fix loop logic to prevent playing extra part when loop is disabled This feature allows multi-part groups to automatically cycle through parts at each scheduled start time, enabling playlist-like behavior for scheduled content.
…red start times - Fix bug where Auto Step incorrectly selected parts when scheduled start times were filtered out - Track occurrence index separately from array index to ensure correct part cycling - Increment occurrence index only when start time passes the filter (startTime >= lastStopTime) - This ensures parts cycle correctly even when some scheduled times are filtered due to currently playing parts Fixes issue where parts would be selected incorrectly based on array position rather than actual occurrence number when start times are filtered.
- Add null check for partToPlay in Auto Step cycling logic - Add null check for firstPlayablePart in non-Auto Step path - Addresses potential Copilot concerns about type safety
|
3 participants
Fixes frame-rate specific duration parsing issue