fix(compositor): improve image overlay quality, caching, aspect ratio, and selectability#78
Conversation
…, and selectability - Replace nearest-neighbor prescaling with bilinear (image crate Triangle filter) for much better rendering of images containing text or fine detail - Cache decoded image overlays across UpdateParams calls — only re-decode when data_base64 or target rect dimensions change, reusing existing Arc<DecodedOverlay> otherwise - Lock aspect ratio for image layers during resize (same as video layers) - Show actual image thumbnail in compositor canvas UI for easier selection; switch border from dotted to solid, remove crosshatch pattern Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Use old_imgs.get(i) instead of old_imgs[i] to avoid a panic when a previous decode_image_overlay call failed, leaving old_imgs shorter than old_cfgs. Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
…ader MIME detection - Build a HashMap<usize, &Arc<DecodedOverlay>> by walking old configs and decoded overlays in tandem, so cache lookups use config index rather than assuming positional alignment (which breaks when a previous decode failed) - Add WebP and GIF magic-byte detection for image thumbnail data URIs Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
…ching Replace incorrect positional index mapping with a content-keyed cache that matches decoded overlays to configs by comparing prescaled bitmap dimensions against the config's target rect. This correctly handles the case where a mid-list decode failure makes the decoded slice shorter than the config vec — failed configs are skipped (not consumed) because their target dimensions won't match the next decoded overlay. Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
…bove video layers Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
| const imgSrc = useMemo(() => { | ||
| if (!overlay.dataBase64) return undefined; | ||
| let mime = 'image/jpeg'; // default fallback | ||
| if (overlay.dataBase64.startsWith('iVBOR')) mime = 'image/png'; | ||
| else if (overlay.dataBase64.startsWith('R0lGOD')) mime = 'image/gif'; | ||
| else if (overlay.dataBase64.startsWith('UklGR')) mime = 'image/webp'; | ||
| return `data:${mime};base64,${overlay.dataBase64}`; | ||
| }, [overlay.dataBase64]); |
There was a problem hiding this comment.
📝 Info: Object URL cleanup correctness under race conditions
The blob URL lifecycle in this useEffect is correctly ordered: the cancelled flag is checked at line 434 before URL.createObjectURL at line 435, so if cleanup runs while the fetch is in-flight, no blob URL is ever created, and no leak occurs. When cleanup runs after the URL was created, url is already set, so URL.revokeObjectURL(url) at line 444 correctly reclaims it. One minor UX note: when dataBase64 changes from one valid value to another, the old blob URL is revoked in cleanup but setImgSrc(undefined) is NOT called, so the <img> element briefly references a revoked URL until the new fetch completes. In practice, fetch on a data URI is near-instantaneous (no network), so this is imperceptible.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
…ression Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
- Replace dimension-matching cache heuristic with index-based mapping using image_overlay_cfg_indices (finding #1) - Only update x/y position on cache hit, not full rect clone (finding #2) - Fix MIME sniffing comment wording to 'base64-encoded magic bytes', add BMP detection (finding #3) - Switch from data-URI to URL.createObjectURL with cleanup for image overlay thumbnails (finding #4) - Change SAFETY comment to Invariant in prescale_rgba (finding #7) Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
…s, optimize base64 decode - Backend: prescale images with aspect-ratio preservation (scale-to-fit instead of stretch-to-fill) and centre within the target rect. - Backend: re-centre cached overlays on position update. - Frontend: detect natural image dimensions on add and set initial rect to match source aspect ratio. - Frontend: add opacity/rotation slider controls for selected image overlays (matching video and text layer controls). - Frontend: fix findAnyLayer to pass through rotationDegrees and zIndex for image overlays instead of hardcoding 0. - Frontend: replace O(n) atob + byte-by-byte loop with fetch(data-URI) for more efficient base64-to-blob conversion. - Frontend: remove BMP MIME detection (inconsistent browser support). - Frontend: add z-index band allocation comments (video 0-99, text 100-199, image 200+). Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
…anvas preview Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
…hange detection Add rotationDegrees and zIndex to the image overlay change-detection comparisons in the params sync effect so that YAML or backend changes to these fields are reflected in the UI. Also add the missing zIndex check to the text overlay change detection for consistency. Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
| m.width !== currentImg[i].width || | ||
| m.height !== currentImg[i].height || | ||
| m.opacity !== currentImg[i].opacity || | ||
| m.rotationDegrees !== currentImg[i].rotationDegrees || | ||
| m.zIndex !== currentImg[i].zIndex || | ||
| m.visible !== currentImg[i].visible |
There was a problem hiding this comment.
🟡 Missing dataBase64 in image overlay sync change detection causes stale thumbnails
The PR added rotationDegrees and zIndex to the image overlay sync comparison block but omitted dataBase64. This means if the image data changes externally (e.g., via YAML editing or backend push) while all other properties remain the same, parseImageOverlays will produce objects with updated dataBase64, but the comparison at ui/src/hooks/useCompositorLayers.ts:432-444 will return false, causing the old state (with stale dataBase64) to be retained. Since this PR also added a dataBase64-dependent thumbnail in ImageOverlayLayer (ui/src/components/CompositorCanvas.tsx:418-446), the thumbnail will show the old image indefinitely until an unrelated property triggers the update.
(Refers to lines 432-444)
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| opacity: o.opacity ?? 1.0, | ||
| rotationDegrees: o.rotation_degrees ?? 0, | ||
| zIndex: o.z_index ?? 0, | ||
| zIndex: o.z_index ?? 200 + i, |
There was a problem hiding this comment.
🚩 Frontend/backend z-index default mismatch for image overlays
The frontend defaults image overlay zIndex to 200 + i in parseImageOverlays at ui/src/hooks/useCompositorLayers.ts:206, but the backend's OverlayTransform defaults z_index to 0 via #[serde(default = "default_z_index")] at crates/nodes/src/video/compositor/config.rs:49. Since z_index: 0 is not nullish, the ?? fallback in parseImageOverlays never triggers for backend-sourced configs. Image overlays created via the frontend UI correctly use 200+, but overlays defined in YAML without explicit z_index will appear at z-index 0 (behind video layers). This is a design inconsistency rather than a bug — YAML authors can set z_index: 200 explicitly.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| cancelled = true; | ||
| if (url) URL.revokeObjectURL(url); | ||
| }; | ||
| }, [overlay.dataBase64]); |
There was a problem hiding this comment.
🚩 Large base64 strings passed through React state and props without memoization
The ImageOverlayState.dataBase64 field (potentially multi-MB) flows through React state (setImageOverlays), is stored in refs (imageOverlaysRef), serialized in serializeImageOverlays, compared in the sync effect, and passed as a prop to ImageOverlayLayer. The useEffect dependency at ui/src/components/CompositorCanvas.tsx:446 triggers on overlay.dataBase64 changes, requiring React to compare potentially large strings on every render. For multi-MB images this could cause performance issues. The fetch(data:...) approach amortizes the decode cost but doesn't address the comparison cost.
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
Addresses four image overlay issues in the video compositor:
Rendering quality: Replaced hand-rolled nearest-neighbor
prescale_rgbawithimage::imageops::resizeusing bilinear (Triangle) filtering. This runs once at config time, so the per-frame blit still hits the identity-scale fast path.Performance/caching:
apply_update_paramsno longer unconditionally re-decodes all image overlays on every config update. It builds a content-keyed cache (HashMap<(data_base64, width, height), Vec<Arc<DecodedOverlay>>>) from successfully decoded old overlays. Each decoded overlay is mapped back to its originating config viaimage_overlay_cfg_indices— a parallelVec<usize>that records the config index at decode time — eliminating the earlier dimension-matching heuristic. When only video layer positions change (the common case), existing bitmaps are reused with only position and visual fields updated.Aspect ratio: Image layers now have locked aspect ratio during resize, matching the existing video layer behavior. The condition in
computeUpdatedLayerwas broadened fromlayerKind === 'video'to include'image'. Additionally, the backend now preserves source aspect ratio during prescaling (scale-to-fit instead of stretch-to-fill), and the frontend detects natural image dimensions on add to set aspect-ratio-correct initial rect sizes.UI selectability: Image overlay layers in the compositor canvas now show an actual thumbnail of the image (via
fetch(data:URI)→URL.createObjectURL) instead of a generic icon + crosshatch pattern. Border changed fromdottedtosolid. UnusedImageBadgeandImageIconcomponents removed.Updates since last revision
Backend aspect-ratio-preserving prescale (bug fix):
decode_image_overlaynow computes scale-to-fit dimensions within the target rect instead of stretching to fill. The overlay rect is adjusted to the fitted dimensions and centred within the original target area. On cache hit, the overlay is re-centred within the potentially repositioned config rect usingcast_signed()arithmetic.Frontend natural dimension detection (bug fix):
handleImageFileChangecreates anImageelement to detectnaturalWidth/naturalHeightand passes them toaddImageOverlay, which computes an aspect-ratio-preserving initial rect (scales to fit within 200px on the largest side).Image layer opacity/rotation controls (bug fix): Added
selectedImageOverlaydetection inUnifiedLayerListwith opacity and rotation sliders matching video and text layer controls. Wired through newonUpdateImageprop threaded fromCompositorNode→UnifiedLayerList→updateImageOverlayhook callback.Canvas rotation preview (bug fix): Added
transform: rotate(${overlay.rotationDegrees}deg)toImageOverlayLayer'sLayerBoxstyle, matching the pattern used byVideoLayerandTextOverlayLayer. Previously, rotation applied in the backend stream output but was not visually reflected in the compositor canvas UI preview.findAnyLayerfix: Image overlays now pass through actualrotationDegreesandzIndexinstead of hardcoding0. This fixes drag/resize behavior for rotated or z-reordered image layers.Overlay sync change detection fix: Added
rotationDegreesandzIndexto the image overlay change-detection comparisons in the params sync effect, so YAML or backend updates to these fields are reflected in the UI. Also added the missingzIndexcheck to text overlay change detection for consistency.fetch()-based base64 decode (performance): Replaced
useMemo+atob()+ byte-by-byte Uint8Array loop withuseEffect+fetch(data:${mime};base64,${b64})for native browser decoding. More efficient for multi-MB images. Properly cleans up blob URLs via effect return.BMP detection removed: Removed
'Qk'prefix check for BMP — inconsistent browser support makes it unreliable.Z-index band documentation: Added comments documenting z-index band allocation (video: 0–99, text: 100–199, image: 200+). Fixed
addImageOverlayandparseImageOverlaysto default to200 + iinstead of0, matching implicit z-index used by layer list display.Review & Testing Checklist for Human
Verify aspect-ratio preservation end-to-end: Upload an image with unusual aspect ratio (e.g., 1920x500 banner). Verify it renders without stretching in the canvas preview. Resize the layer — should maintain aspect ratio like video layers. Check that YAML
image_overlays[0].transform.rect.width/heightreflect the config dimensions, not the fitted dimensions (the backend centers the fitted bitmap within the rect).Test opacity/rotation controls for image layers: Select an image overlay in the layer list. Verify opacity and rotation sliders appear and respond. Drag slider, release, verify YAML updates with correct
opacity/rotation_degreesvalues. Test with multiple image overlays to ensure selection state tracks correctly.Test rotation visual feedback in canvas preview: Add an image overlay and rotate it using the rotation slider (e.g., 45°). Verify the image layer box in the canvas preview visually rotates to match. The rotation should be visible immediately as you drag the slider, not just in the output stream. Compare with video/text layer rotation behavior for consistency.
Test overlay sync with YAML edits: Add an image overlay, then manually edit its
rotation_degreesorz_indexin YAML and apply the change. Verify the canvas preview updates correctly after the 3-second commit guard expires. The rotation should reflect in the canvas transform and the z-index should update layer stacking order.Test z-index interaction: Add image overlay over full-canvas background video. Verify image is directly clickable without moving background. Add multiple overlays, verify z-index displays correctly in layer list (
z:200,z:201, etc.) and YAML round-trips correctly.Test
findAnyLayerfix: Add image overlay, rotate it 45° and move it. Then drag it — verify rotation is preserved during drag (the layer shouldn't snap back to 0° rotation). Repeat with z-index changes.Test fetch() decode & cleanup: Open DevTools memory profiler. Add several large (2-4 MB) image overlays. Remove them or change their base64 content. Verify blob URLs are being revoked (check Network tab for blob: URLs, or memory doesn't grow indefinitely).
Test cache re-centering: Add an image overlay with unusual aspect ratio. Move it to a new position in YAML (edit
rect.x/y). Verify the bitmap is re-centered correctly within the new position on re-render (shouldn't jump or misalign).Notes
DecodedOverlay.rect, which may differ from the config rect dimensions. The config rect acts as the "container" and the overlay rect is the "fitted content" within it. Per-frame blitting uses the overlay rect, hitting the identity-scale fast path.Devin session: https://staging.itsdev.in/sessions/a1aba416ded648f086c490c403b17d93
Requested by: @streamer45