fix(layout): apply the webcam layout only to clips that have a camera - #261
Merged
EtienneLescot merged 2 commits intoAug 4, 2026
Merged
Conversation
…s clips The layout preset is global — one panel for the whole timeline — but the camera is per clip: a project mixes a screen+webcam recording with a plain import without one. `LiveParams::has_webcam` already carried that distinction, but only `live.rs` derived it, so the preview was right and every export was wrong. An export sets its `LiveParams` once for the whole timeline (`compositor-view-napi`), keeping the `true` default. And `ExportDialog` sends the SCREEN path as `webcamPath` when a clip has no camera, purely so the decoder has something valid to open — so the PiP box was drawn with the screen recording behind it, duplicated into its own corner. That is the mirror reported in #248, which the greyed-out Layout panel (correctly gated on `hasAnyClipWithCamera`) then left no way to turn off. `webcam_is_real` moves next to the field it decides, and `walk_composited_timeline` — shared by MP4 and GIF on all three backends — rebinds it per clip. A targeted `set_has_webcam` rather than a per-clip `set_live_params`, which would clobber the settings the caller posted. Verified on a real export (`run_composited_multi`, h264_amf) with the camera path equal to the screen path: the thumbnail is gone. The bench gains a `--webcam` override because the no-camera case is not different *content* but an identical *path*, and cannot be replayed otherwise. Refs #248
The preset is global — one panel for the whole timeline — but the camera is per clip. `layoutByClip` already carried a resolved layout per visible clip; it just never asked whether that clip had a camera, so every clip got the preset whether or not it had anything to put in it. Gating the camera's draw (`has_webcam`, previous commit) is not enough, and this is the half that actually shows. The block presets — `dual-frame`, `vertical-stack` — size the SCREEN off the block: they reserve the camera's half of the frame. A camera-less clip therefore kept its screen squeezed into that half with nothing beside it, which no draw-time gate can undo. Under picture-in-picture the defect is invisible, since the screen stays full-frame there and only a thumbnail is added. So a clip with no camera now lays out as if the preset were "no-webcam" — which `computeCompositeLayout` already implements (full-frame screen, no webcam rect). The predicate matches the `webcamPath` sent with the clip exactly, so the layout and the decoder cannot disagree. It is deliberately NOT `hasAnyClipWithCamera`, which gates the Layout panel and ignores `visible` on purpose so the panel stays reachable to un-hide a camera. `PreviewCanvas` had the same hole, and its own comment named it: it hid the webcam SLOT for a camera-less clip but left the screen geometry alone. Five regression tests, all of which fail on the parent commit: one per preset for "only the clip that has a camera gets a webcam rect", and one per block preset for "the camera-less clip gets its full frame back". The existing webcamRect test asserted a PiP rect for an asset with no camera — that premise was the bug, so its fixture gains the camera its preset presupposes and it goes on testing the px→fraction conversion it describes. Refs #248
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
EtienneLescot
added a commit
that referenced
this pull request
Aug 5, 2026
Every RC of a line shipped the same release body. The notes start tag was derived from the stable version, so v1.9.0-rc.1 and v1.9.0-rc.2 both spanned v1.8.0..<tag> — rc.2 just repeated rc.1's list plus its own few entries, and v1.8.0-rc.8 and rc.9 came out byte-identical. Testers had no way to see what a re-cut actually changed, which is the one question an RC body has to answer. Resolve the previous RC of the same line instead, walking down from the current rc number so a skipped or failed RC doesn't break the chain. rc.1 still falls back to the previous stable, and stable releases are untouched. Build the RC body from `git log` rather than --generate-notes. GitHub's generator lists only the PRs it manages to associate and silently drops real ones: #254 and #261 were merged into release/v1.9.0 yet never appeared in v1.9.0-rc.2's body, so an RC could omit the very fix it was cut for. The commit range is the actual diff. Stable releases keep --generate-notes — they are the public-facing ones and want the PR links and the New Contributors section. Needs fetch-depth: 0 on the publish job's checkout for the tags and history.
EtienneLescot
added a commit
that referenced
this pull request
Aug 5, 2026
Every RC of a line shipped the same release body. The notes start tag was derived from the stable version, so v1.9.0-rc.1 and v1.9.0-rc.2 both spanned v1.8.0..<tag> — rc.2 just repeated rc.1's list plus its own few entries, and v1.8.0-rc.8 and rc.9 came out byte-identical. Testers had no way to see what a re-cut actually changed, which is the one question an RC body has to answer. Resolve the previous RC of the same line instead, walking down from the current rc number so a skipped or failed RC doesn't break the chain. rc.1 still falls back to the previous stable, and stable releases are untouched. Build the RC body from `git log` rather than --generate-notes. GitHub's generator lists only the PRs it manages to associate and silently drops real ones: #254 and #261 were merged into release/v1.9.0 yet never appeared in v1.9.0-rc.2's body, so an RC could omit the very fix it was cut for. The commit range is the actual diff. Stable releases keep --generate-notes — they are the public-facing ones and want the PR links and the New Contributors section. Needs fetch-depth: 0 on the publish job's checkout for the tags and history.
EtienneLescot
added a commit
that referenced
this pull request
Aug 5, 2026
Every RC of a line shipped the same release body. The notes start tag was derived from the stable version, so v1.9.0-rc.1 and v1.9.0-rc.2 both spanned v1.8.0..<tag> — rc.2 just repeated rc.1's list plus its own few entries, and v1.8.0-rc.8 and rc.9 came out byte-identical. Testers had no way to see what a re-cut actually changed, which is the one question an RC body has to answer. Resolve the previous RC of the same line instead, walking down from the current rc number so a skipped or failed RC doesn't break the chain. rc.1 still falls back to the previous stable, and stable releases are untouched. Build the RC body from `git log` rather than --generate-notes. GitHub's generator lists only the PRs it manages to associate and silently drops real ones: #254 and #261 were merged into release/v1.9.0 yet never appeared in v1.9.0-rc.2's body, so an RC could omit the very fix it was cut for. The commit range is the actual diff. Stable releases keep --generate-notes — they are the public-facing ones and want the PR links and the New Contributors section. Needs fetch-depth: 0 on the publish job's checkout for the tags and history.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #248.
The rule, in one line: the layout preset is global — one panel for the whole timeline — but the camera is per clip, so the preset must only apply to the clips that actually have one.
The Layout panel already got the first half of that right on
release/v1.9.0:LayoutPanedisables every control unlesshasAnyClipWithCamera(assets, clips), so it's available as soon as one clip has a camera and greyed out when none does. That part needed no change. What was missing is the second half — the preset was being forced onto the clips that have no camera. Two independent bugs, one in each renderer.1. The export drew the screen inside the PiP box
LiveParams::has_webcamalready existed for this, but onlylive.rsderived it, so the live preview was right and every export was wrong:LiveParamsonce for the whole timeline (compositor-view-napi), keeping thetruedefault;ExportDialogsends the screen path aswebcamPathfor a camera-less clip, purely so the decoder has something valid to open.So the box was drawn with the screen recording decoding behind it — the recording duplicated into its own corner. And because the Layout panel is (correctly) greyed out with no camera, the reporter had no way to switch the preset off. Hence "I had to set up my phone as a webcam to unlock the panel".
webcam_is_realmoves fromlive.rstoframe_geometry.rs, next to the field it decides, andwalk_composited_timeline— the walk shared by MP4 and GIF across all three backends — rebinds it per clip. A targetedset_has_webcamrather than a per-clipset_live_params, which would clobber whatever the caller already posted.2. The block presets kept reshaping the screen — preview and export
Gating the camera's draw is not enough, and this is the half that actually shows.
dual-frameandvertical-stacksize the screen off the block: they reserve the camera's half of the frame. A camera-less clip therefore kept its screen squeezed into that half with nothing beside it, which no draw-time gate can undo. Under picture-in-picture it's invisible — the screen stays full-frame there and only a thumbnail is added.layoutByClipalready carried a resolved layout per visible clip; it simply never asked whether that clip had a camera. So a camera-less clip now lays out as if the preset were"no-webcam"— whichcomputeCompositeLayoutalready implements (full-frame screen, no webcam rect). The predicate matches thewebcamPathsent with the clip exactly, so the layout and the decoder cannot disagree. It is deliberately nothasAnyClipWithCamera, which gates the panel and ignoresvisibleon purpose so the panel stays reachable to un-hide a camera.PreviewCanvashad the same hole, and its own comment named it: it hid the webcam slot for a camera-less clip but left the screen geometry alone.Verification
Export, real path (
run_composited_multi→h264_amf), camera path equal to the screen path — the production case exactly: before, the screen recording sits in a rounded PiP box bottom-right; after, nothing is drawn.The bench gains that
--webcamoverride because the no-camera case is not different content but an identical path — my first attempt used two files with identical pixels and measured a 0 px diff, becausewebcam_is_realcompares paths and was right to say "yes, that's a camera".Layout, 5 new regression tests, all confirmed failing on the parent commit: one per preset for "only the clip that has a camera gets a webcam rect", and one per block preset for "the camera-less clip gets its full frame back". The existing
webcamRecttest asserted a PiP rect for an asset with no camera — that premise was the bug, so its fixture gains the camera its preset presupposes and goes on testing the px→fraction conversion it describes.Confirmed by hand in the app on a mixed timeline (one imported clip with no camera + one recording with one) under side-by-side.
1622 JS tests pass (6 pre-existing failures in
webm-seek-index.test.ts, unrelated — they fail identically on the parent commit); 122 compositor unit tests pass;tsc --noEmit, biome, and clippy clean on the changed lines.Deliberately out of scope
no-webcamto be the default preset when no camera is connected. Cosmetic now that both renderers are gated — the preset no longer changes anything for a camera-less clip — but the panel still reads misleadingly. Happy to add it.ExportDialogignorescameraTrack.visiblewheresceneDescriptionhonours it. Unreachable today: no UI path writesvisible: false, and a recording made without a camera getscameraTrack: null.