Improved rendering engine, recording resilience, and analytics trackingRendering engine tweaks#1674
Conversation
Made-with: Cursor
…btraction Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
… simulation Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
…d op Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
…pers Made-with: Cursor
…mentations Made-with: Cursor
…decar Made-with: Cursor
Made-with: Cursor
…ecovery Made-with: Cursor
…cycle Made-with: Cursor
…breakdown Made-with: Cursor
…zoom_segments Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
…ate encoder finish logic Made-with: Cursor
…buffer rebuild Made-with: Cursor
…olation Made-with: Cursor
…n range Made-with: Cursor
Made-with: Cursor
…thOverlay Made-with: Cursor
|
@greptileai please re-review the PR |
| let kernel_size = 21; | ||
| let k = kernel_size - 1; | ||
| let offset_base = -vel_len / 2.0 / vel_len - 0.5; | ||
|
|
There was a problem hiding this comment.
offset_base always simplifies to -1.0
The expression:
let offset_base = -vel_len / 2.0 / vel_len - 0.5;cancels to -(vel_len / 2.0) / vel_len - 0.5 = -0.5 - 0.5 = -1.0 regardless of vel_len. The loop then samples from input.uv - velocity_uv (i=0) to input.uv - 0.05 * velocity_uv (i=19), placing all 20 trailing samples strictly behind the cursor with no forward coverage.
If the intent was to center the kernel symmetrically (samples from -0.5*velocity_uv to +0.5*velocity_uv), offset_base should be -0.5. If the intent was to trail behind the cursor (samples from -velocity_uv to 0.0), offset_base should simply be -1.0.
Either way, using the current expression with vel_len terms that cancel is misleading and masks the actual behavior. Suggest replacing with the explicit constant:
| let offset_base = -1.0; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/rendering/src/shaders/cursor.wgsl
Line: 97
Comment:
**`offset_base` always simplifies to `-1.0`**
The expression:
```wgsl
let offset_base = -vel_len / 2.0 / vel_len - 0.5;
```
cancels to `-(vel_len / 2.0) / vel_len - 0.5 = -0.5 - 0.5 = -1.0` regardless of `vel_len`. The loop then samples from `input.uv - velocity_uv` (i=0) to `input.uv - 0.05 * velocity_uv` (i=19), placing all 20 trailing samples strictly behind the cursor with no forward coverage.
If the intent was to center the kernel symmetrically (samples from `-0.5*velocity_uv` to `+0.5*velocity_uv`), `offset_base` should be `-0.5`. If the intent was to trail behind the cursor (samples from `-velocity_uv` to `0.0`), `offset_base` should simply be `-1.0`.
Either way, using the current expression with `vel_len` terms that cancel is misleading and masks the actual behavior. Suggest replacing with the explicit constant:
```suggestion
let offset_base = -1.0;
```
How can I resolve this? If you propose a fix, please make it concise.| view: currentTexture.createView(), | ||
| clearValue: { r: 0, g: 0, b: 0, a: 1 }, | ||
| loadOp: "clear", | ||
| loadOp: "load", |
There was a problem hiding this comment.
loadOp: "load" may expose stale pixels on first frame
Switching from "clear" to "load" skips resetting the attachment to clearValue before the render pass. In WebGPU, on the very first frame (or after a resize/GPU-device-lost recovery), the texture contents are undefined — "load" will then composite on top of garbage memory rather than a clean black background. This can manifest as single-frame visual glitches during playback start or after seeking.
The same change is applied to both renderFrameWebGPU (line 301) and renderNv12FrameWebGPU (line 415).
If the goal is to avoid the clear cost when the composite shader always writes every pixel, an alternative is to keep "clear" but only for the first frame render of a new session. Otherwise, at minimum add a comment confirming the shader guarantees full-pixel coverage so future readers understand the invariant.
| loadOp: "load", | |
| loadOp: "clear", | |
| clearValue: { r: 0, g: 0, b: 0, a: 1 }, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/utils/webgpu-renderer.ts
Line: 302
Comment:
**`loadOp: "load"` may expose stale pixels on first frame**
Switching from `"clear"` to `"load"` skips resetting the attachment to `clearValue` before the render pass. In WebGPU, on the very first frame (or after a resize/GPU-device-lost recovery), the texture contents are **undefined** — `"load"` will then composite on top of garbage memory rather than a clean black background. This can manifest as single-frame visual glitches during playback start or after seeking.
The same change is applied to both `renderFrameWebGPU` (line 301) and `renderNv12FrameWebGPU` (line 415).
If the goal is to avoid the clear cost when the composite shader always writes every pixel, an alternative is to keep `"clear"` but only for the first frame render of a new session. Otherwise, at minimum add a comment confirming the shader guarantees full-pixel coverage so future readers understand the invariant.
```suggestion
loadOp: "clear",
clearValue: { r: 0, g: 0, b: 0, a: 1 },
```
How can I resolve this? If you propose a fix, please make it concise.| view: context.getCurrentTexture().createView(), | ||
| clearValue: { r: 0, g: 0, b: 0, a: 1 }, | ||
| loadOp: "clear", | ||
| loadOp: "load", |
There was a problem hiding this comment.
Same
loadOp: "load" concern for Nv12 path
This is the same "clear" → "load" change applied to renderNv12FrameWebGPU. If the composite pass does not guarantee full pixel coverage on the first frame, the Nv12 render path will have the same stale-pixel risk.
| loadOp: "load", | |
| loadOp: "clear", | |
| clearValue: { r: 0, g: 0, b: 0, a: 1 }, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/utils/webgpu-renderer.ts
Line: 415
Comment:
**Same `loadOp: "load"` concern for Nv12 path**
This is the same `"clear"` → `"load"` change applied to `renderNv12FrameWebGPU`. If the composite pass does not guarantee full pixel coverage on the first frame, the Nv12 render path will have the same stale-pixel risk.
```suggestion
loadOp: "clear",
clearValue: { r: 0, g: 0, b: 0, a: 1 },
```
How can I resolve this? If you propose a fix, please make it concise.
recording-diagnostics.jsonsidecar.copy_with_new_timing.auth_surfaceproperty to all auth tracking events, fixis_signupflags on login forms, add a 7-day window guard for signup tracking, and correct the Stripe webhook platform field to a proper enum.Greptile Summary
This is a broad, well-scoped improvement PR touching the rendering engine, recording resilience, editor playback, decoder, encoder, and analytics layers. The changes are generally sound and well-tested, but two issues in the rendering pipeline need attention before merge.
Key changes:
partition_point.recording-diagnostics.jsonsidecar instead of aborting the entire recording. Encoder failures now propagate as errors rather than warn-and-continue. Shared helpers extracted for blocking-thread-finish and mux-send-error patterns.copy_with_new_timing, fixing writer failures on camera overlay tracks.best_idvsdecoder_idxmismatch).auth_surfaceadded to all auth tracking events;is_signupflags corrected on login forms; 7-day signup tracking window guard; Stripe webhookplatformfield corrected to a proper enum.loadOp: "load"inwebgpu-renderer.ts: Both render paths changed from"clear"to"load", which risks undefined/stale pixels on the first frame or after a GPU device-lost event.cursor.wgsloffset_base: The expressionoffset_base = -vel_len / 2.0 / vel_len - 0.5always simplifies to-1.0, making the motion blur sampling range a fixed constant. This is likely unintentional and could produce incorrect visual results depending on the design intent.Confidence Score: 3/5
loadOp: "load"change risks first-frame artifacts in the WebGPU renderer for both playback paths, and theoffset_baseexpression incursor.wgslalways evaluates to-1.0regardless of velocity, which may or may not be the intended blur range but is at minimum misleading and could produce incorrect visual output.apps/desktop/src/utils/webgpu-renderer.ts(both loadOp changes) andcrates/rendering/src/shaders/cursor.wgsl(offset_base expression).Important Files Changed
offset_baseexpression always simplifies to-1.0due tovel_lencancellation, making the sampling range a constant regardless of intent.loadOpchanged from"clear"to"load"on both render paths; risks stale/garbage pixels on first frame render or after GPU device loss events.partition_point.recording-diagnostics.jsonsidecar; well-tested.decoder_count, preventing out-of-range access; fixesbest_idvsdecoder_idxmismatch inupdate_decoder_position.click_springpropagation to cursor interpolation.CURRENT_DATE()to a JS-side 7-day window guard; addsgetAffectedRowshelper for cross-ORM result shape compatibility.platformfield corrected from a boolean expression to a proper `"desktop"Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Recording Input] --> B{Track Type} B -->|Display - required| C[Display Pipeline] B -->|Mic/Camera/System - optional| D[Optional Track Pipelines] C --> E{Display Error?} E -->|Yes| F[Abort Recording] E -->|No| G[Continue] D --> H{Track Error?} H -->|Yes - Runtime| I[record_track_failure\nRuntime stage] H -->|Yes - Stop| J[finalize_optional_track\nStop stage] H -->|No| K[Track finished OK] I --> L[SharedTrackFailures\nArc-Mutex-Vec] J --> L K --> G G --> M[stop_recording] M --> N[take_track_failures] N --> O{Any failures?} O -->|Yes| P[Write recording-diagnostics.json] O -->|No| Q[Clean finish] P --> Q subgraph Cursor Rendering R[CursorEvents] --> S[decimate_cursor_moves] S --> T[build_smoothed_timeline\nFixed-timestep 60fps loop] T --> U[Click lookahead target\n500ms window] T --> V[Spring profile\nbinary search] U --> W[interpolate_timeline\nIndex lookup] V --> W W --> X[InterpolatedCursorPosition] endPrompt To Fix All With AI
Last reviewed commit: "fix(web): handle arr..."