feat(desktop): add clip transitions and capture-area improvements#2013
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| let transition_position = self | ||
| .transitions | ||
| .partition_point(|transition| transition.segment_index as usize <= segment_index); | ||
| let transition = self.transitions.get(transition_position.checked_sub(1)?)?; | ||
| if transition.segment_index as usize != segment_index { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
effective_transition relies on partition_point, which assumes self.transitions is sorted by segment_index. That’s true after deserializing, but since transitions is public it’s easy for in-memory mutations to accidentally break that invariant.
A simple scan keeps this robust and the list should stay tiny anyway:
| let transition_position = self | |
| .transitions | |
| .partition_point(|transition| transition.segment_index as usize <= segment_index); | |
| let transition = self.transitions.get(transition_position.checked_sub(1)?)?; | |
| if transition.segment_index as usize != segment_index { | |
| return None; | |
| } | |
| let transition = self | |
| .transitions | |
| .iter() | |
| .rev() | |
| .find(|transition| transition.segment_index as usize == segment_index)?; |
| .await | ||
| .map_err(|e| format!("Failed to render frame: {e}"))?; | ||
| let frame = if let Some((outgoing, kind, progress)) = transition_mapping { | ||
| let outgoing_segment = &render_segments[outgoing.segment.recording_clip as usize]; |
There was a problem hiding this comment.
This index access can panic if recording_clip isn’t a dense 0..N mapping into render_segments (or if the timeline gets out of sync). Seems safer to turn it into a nice error like the other decode/render failure paths.
| let outgoing_segment = &render_segments[outgoing.segment.recording_clip as usize]; | |
| let outgoing_segment = render_segments | |
| .get(outgoing.segment.recording_clip as usize) | |
| .ok_or_else(|| { | |
| format!( | |
| "Missing render segment for recording_clip={}", | |
| outgoing.segment.recording_clip | |
| ) | |
| })?; |
| } | ||
| } | ||
|
|
||
| pub fn ensure_size(&mut self, device: &wgpu::Device, width: u32, height: u32) { |
There was a problem hiding this comment.
Minor edge case: wgpu will generally error if you try to create a 0x0 texture (can happen during resize/minimize). A small guard here avoids surprising failures.
| pub fn ensure_size(&mut self, device: &wgpu::Device, width: u32, height: u32) { | |
| if width == 0 || height == 0 { | |
| self.textures = None; | |
| return; | |
| } |
richiemcilroy
changed the title
|
hey @greptileai, please re-review the PR |
| import type { | ||
| PersistenceSyncAPI, | ||
| PersistenceSyncData, | ||
| } from "@solid-primitives/storage"; | ||
| import type { CropBounds, Ratio } from "~/components/Cropper"; | ||
| import type { DisplayId } from "~/utils/tauri"; | ||
|
|
||
| export const AREA_SELECTION_STORAGE_KEY = "target-select-area-preferences-v1"; | ||
| export const AREA_SELECTION_STORAGE_SYNC: PersistenceSyncAPI = [ | ||
| (subscriber) => | ||
| window.addEventListener("storage", (event) => { | ||
| const data = areaSelectionSyncData(event); | ||
| if (data) subscriber(data); | ||
| }), | ||
| () => {}, | ||
| ]; |
There was a problem hiding this comment.
PersistenceSyncAPI's update hook is currently a no-op, so multiple makePersisted(...) consumers in the same window won't be notified (since storage events don't fire on the same document), and each subscriber adds another storage listener.
| import type { | |
| PersistenceSyncAPI, | |
| PersistenceSyncData, | |
| } from "@solid-primitives/storage"; | |
| import type { CropBounds, Ratio } from "~/components/Cropper"; | |
| import type { DisplayId } from "~/utils/tauri"; | |
| export const AREA_SELECTION_STORAGE_KEY = "target-select-area-preferences-v1"; | |
| export const AREA_SELECTION_STORAGE_SYNC: PersistenceSyncAPI = [ | |
| (subscriber) => | |
| window.addEventListener("storage", (event) => { | |
| const data = areaSelectionSyncData(event); | |
| if (data) subscriber(data); | |
| }), | |
| () => {}, | |
| ]; | |
| import type { | |
| PersistenceSyncAPI, | |
| PersistenceSyncCallback, | |
| PersistenceSyncData, | |
| } from "@solid-primitives/storage"; | |
| import type { CropBounds, Ratio } from "~/components/Cropper"; | |
| import type { DisplayId } from "~/utils/tauri"; | |
| export const AREA_SELECTION_STORAGE_KEY = "target-select-area-preferences-v1"; | |
| const areaSelectionSubscribers = new Set<PersistenceSyncCallback>(); | |
| let areaSelectionListening = false; | |
| export const AREA_SELECTION_STORAGE_SYNC: PersistenceSyncAPI = [ | |
| (subscriber) => { | |
| areaSelectionSubscribers.add(subscriber); | |
| if (areaSelectionListening) return; | |
| areaSelectionListening = true; | |
| window.addEventListener("storage", (event) => { | |
| const data = areaSelectionSyncData(event); | |
| if (!data) return; | |
| for (const sub of areaSelectionSubscribers) sub(data); | |
| }); | |
| }, | |
| (key, newValue) => { | |
| const data: PersistenceSyncData = { | |
| key, | |
| newValue, | |
| timeStamp: performance.now(), | |
| }; | |
| for (const sub of areaSelectionSubscribers) sub(data); | |
| }, | |
| ]; |
| export type TargetUnderCursor = { display_id: DisplayId | null; window: WindowUnderCursor | null } | ||
| export type TextSegment = { start: number; end: number; track?: number; enabled?: boolean; content?: string; center?: XY<number>; size?: XY<number>; fontFamily?: string; fontSize?: number; fontWeight?: number; italic?: boolean; color?: string; fadeDuration?: number } | ||
| export type TimelineConfiguration = { segments: TimelineSegment[]; zoomSegments: ZoomSegment[]; sceneSegments?: SceneSegment[]; maskSegments?: MaskSegment[]; textSegments?: TextSegment[]; captionSegments?: CaptionTrackSegment[]; keyboardSegments?: KeyboardTrackSegment[]; audioSegments?: AudioTrackSegment[] } | ||
| export type TimelineConfiguration = { segments: TimelineSegment[]; transitions: ClipTransition[]; zoomSegments: ZoomSegment[]; sceneSegments?: SceneSegment[]; maskSegments?: MaskSegment[]; textSegments?: TextSegment[]; captionSegments?: CaptionTrackSegment[]; keyboardSegments?: KeyboardTrackSegment[]; audioSegments?: AudioTrackSegment[] } |
There was a problem hiding this comment.
TimelineConfiguration.transitions is marked required here, but the Rust config skips serializing transitions when empty. Most existing projects will load a timeline without this field, and a lot of the editor code already uses timeline.transitions ?? [].
| export type TimelineConfiguration = { segments: TimelineSegment[]; transitions: ClipTransition[]; zoomSegments: ZoomSegment[]; sceneSegments?: SceneSegment[]; maskSegments?: MaskSegment[]; textSegments?: TextSegment[]; captionSegments?: CaptionTrackSegment[]; keyboardSegments?: KeyboardTrackSegment[]; audioSegments?: AudioTrackSegment[] } | |
| export type TimelineConfiguration = { segments: TimelineSegment[]; transitions?: ClipTransition[]; zoomSegments: ZoomSegment[]; sceneSegments?: SceneSegment[]; maskSegments?: MaskSegment[]; textSegments?: TextSegment[]; captionSegments?: CaptionTrackSegment[]; keyboardSegments?: KeyboardTrackSegment[]; audioSegments?: AudioTrackSegment[] } |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
hey @greptileai, please re-review the PR |
|
hey @greptileai, please re-review the PR |
| .await | ||
| .map_err(|e| format!("Failed to render frame: {e}"))?; | ||
| let frame = if let Some((outgoing, kind, progress)) = transition_mapping { | ||
| let outgoing_media = &editor.segment_medias[outgoing.segment.recording_clip as usize]; |
There was a problem hiding this comment.
This index access can panic if recording_clip ever isn’t a dense 0..N mapping into segment_medias (or if the timeline gets out of sync). Feels nicer to return a structured error like the other decode/render paths.
| let outgoing_media = &editor.segment_medias[outgoing.segment.recording_clip as usize]; | |
| let outgoing_media = editor | |
| .segment_medias | |
| .get(outgoing.segment.recording_clip as usize) | |
| .ok_or_else(|| { | |
| format!( | |
| "Missing segment media for recording_clip={}", | |
| outgoing.segment.recording_clip | |
| ) | |
| })?; |
| is_initial_frame: bool, | ||
| fps: u32, | ||
| ) -> Option<DecodedSegmentFrames> { | ||
| let render_segment = &render_segments[source.segment.recording_clip as usize]; |
There was a problem hiding this comment.
Minor safety thing: this can panic if recording_clip doesn’t line up with render_segments (even if that’s “not supposed to happen”). Since this fn already returns Option, it can just fall back cleanly.
| let render_segment = &render_segments[source.segment.recording_clip as usize]; | |
| let render_segment = render_segments.get(source.segment.recording_clip as usize)?; |
|
hey @greptileai, please re-review the PR |
1 participant
Summary
Validation
pnpm --dir apps/desktop exec tsc --noEmitpnpm --dir apps/desktop exec vitest run src/routes/editor/clip-transitions.test.ts src/routes/editor/timeline-utils.ts src/routes/editor/captions.ts src/routes/editor/captions-export.ts(27 passed)pnpm --dir apps/desktop exec vitest run src/utils/area-selection.test.ts(6 passed)cargo check -p cap-rendering -p cap-editor -p cap-export -p cap-desktop --libcargo clippy -q -p cap-project -p cap-rendering -p cap-editor -p cap-export --all-targets -- -D warningscargo test -q -p cap-project --lib(39 passed)cargo test -q -p cap-rendering --lib(117 passed, 1 ignored)cargo test -q -p cap-editor -p cap-export --lib(27 passed)cargo test -q -p cap-desktop fake_window::tests --lib(12 passed)cap-desktopplayback verification for transition orientation and smoothnessGreptile Summary
This PR introduces clip transitions (cross-fade and fade-through-black) across preview, playback, and export, alongside a locked area-selection feature with quick aspect-ratio controls for the capture overlay. It is a substantial, multi-layer change touching the GPU compositor, audio mixer, Rust project model, playback prefetcher, and SolidJS editor context.
ClipTransitiontype is added toTimelineConfiguration;get_frame_mappingdrives all rendering paths; the GPU compositor (transition.rs), audio cross-fade (audio.rs), and prefetch pipeline (playback.rs) all consume the sameTimelineFrameMappingenum, keeping the fast-path (no transitions) zero-cost.AREA_SELECTION_STORAGE_SYNCnow guards against multiplewindow.addEventListenercalls via asubscribedflag (resolving the pre-existing listener-accumulation concern), and locked bounds are persisted with a 180 ms debounce.cutClipSegmentsForRangeandrippleDeleteAllTracksare transition-aware; caption and transcript deletion guards against cutting through a transition zone.Confidence Score: 5/5
Safe to merge. The change is large but well-validated: 33 new tests across Rust and TypeScript, manual playback verification, and clippy/tsc clean. The one inconsistency (direct slice indexing in two export preview functions vs. the safe .get() used in preview.rs) would produce an unhelpful panic on a malformed project rather than a clean error, but does not affect correct projects.
All rendering paths (GPU, audio, zoom-spring, prefetch, export, captions, transcript) have been updated consistently to route through get_frame_mapping; the no-transition fast paths are preserved with explicit early returns; the subscribed guard in AREA_SELECTION_STORAGE_SYNC fixes the pre-existing listener accumulation issue; and extensive test coverage covers edge cases like fractional sample boundaries, audio midpoint silence, deduplication of transition JSON, and transition-aware cut geometry.
apps/desktop/src-tauri/src/export.rs — two preview functions access render_segments[recording_clip as usize] without bounds checking.
Important Files Changed
Comments Outside Diff (1)
apps/desktop/src/routes/editor/timeline-utils.ts, line 1120-1128 (link)The
importblock forClipTransition,clipTimelineDuration,clipTimelineOffsets,transitionsAfterClipDelete, andtransitionsAfterClipSplitis placed at the end of the file, after theif (import.meta.vitest)test block. While ESM hoisting makes this work at runtime, staticimportdeclarations are conventionally placed at the top of the module — this would violateimport/firstlint rules and makes the dependency graph harder to follow. Move this import block to the top of the file alongside the other imports.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (5): Last reviewed commit: "fix(recording): satisfy range clippy lin..." | Re-trigger Greptile