Dual camera timelapse functionality#198
Open
chris-red wants to merge 2 commits intomainsail-crew:mainfrom
Open
Dual camera timelapse functionality#198chris-red wants to merge 2 commits intomainsail-crew:mainfrom
chris-red wants to merge 2 commits intomainsail-crew:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds dual-camera support to the timelapse component: new config options, async retrieval of the second camera config, per-frame camera-specific transforms, active-camera selection by frame interval, and extended macro/G-code variables for dual-camera control. Changes
Sequence DiagramsequenceDiagram
participant Client as Frame Capture
participant Selector as Active Camera Selector
participant Config as Webcam2 Config Fetcher
participant Transformer as Per-Frame Transformer
participant Renderer as Render Pipeline
Client->>Selector: newframe -> request active camera
Selector->>Selector: compute active index (interval, frame count)
alt webcam2 config needed
Selector->>Config: async getWebcam2Config()
Config-->>Selector: return camera2 settings
end
Client->>Transformer: provide framefile + camera_num
Transformer->>Transformer: check _cameras_have_different_transforms()
Transformer->>Transformer: _build_filter_for_camera(camera_num)
Transformer->>Transformer: apply_frame_transform(framefile, camera_num)
Transformer->>Renderer: mark per_frame_transforms_applied
Renderer->>Renderer: skip global transforms if per-frame applied
Renderer-->>Client: return rendered/transformed frame
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
component/timelapse.py (2)
416-423:⚠️ Potential issue | 🟡 MinorInclude dual-camera keys in G-code-sync trigger list.
Updates to
dual_camera,camera2, andcamera_switch_intervalcurrently do not setgcodechange=True, so macro variables can stay stale after settings POSTs.🔁 Proposed fix
settingsWithGcodechange = [ 'enabled', 'parkhead', 'parkpos', 'park_custom_pos_x', 'park_custom_pos_y', 'park_custom_pos_dz', 'park_travel_speed', 'park_retract_speed', 'park_extrude_speed', 'park_retract_distance', - 'park_extrude_distance', 'park_time', 'fw_retract' + 'park_extrude_distance', 'park_time', 'fw_retract', + 'dual_camera', 'camera2', 'camera_switch_interval' ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@component/timelapse.py` around lines 416 - 423, The settings list named settingsWithGcodechange used to mark which POSTed settings should trigger gcodechange currently omits the dual-camera related keys; add 'dual_camera', 'camera2', and 'camera_switch_interval' to settingsWithGcodechange so updates to those settings set gcodechange=True and macro variables are refreshed—locate the settingsWithGcodechange definition and append these three keys to the array.
500-517:⚠️ Potential issue | 🟠 MajorQuote and escape the camera name in
DUAL_CAMERA_NAMEparameter.
camera2is a user-configurable camera name string that can contain spaces or quotes. Without quoting, names like "USB Camera 1" break Klipper's shlex parameter parsing. The parameter must be quoted and internal quotes escaped.camera2_name = str(self.config['camera2']).replace("'", "\\'")Then update line 516:
+ f" DUAL_CAMERA_NAME='{camera2_name}'" \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@component/timelapse.py` around lines 500 - 517, The DUAL_CAMERA_NAME value coming from self.config['camera2'] must be escaped and quoted to avoid breaking Klipper shlex parsing; create a sanitized camera2_name by converting self.config['camera2'] to a string, replacing any internal single-quote characters with an escaped version, then use that sanitized camera2_name when building gcommand so the DUAL_CAMERA_NAME parameter is wrapped in single quotes (update the gcommand construction where DUAL_CAMERA_NAME is appended to reference the sanitized camera2_name instead of raw self.config['camera2']).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@component/timelapse.py`:
- Around line 285-287: When camera2 is disabled or missing (the checks using
self.config['dual_camera'] and self.config['camera2']), clear any runtime state
for camera2 instead of simply returning so stale values (e.g. snapshoturl2 and
any transform-related fields) can't persist; update both places shown (the early
return around the first check and the similar branch at lines ~306-310) to
explicitly reset snapshoturl2 and transform-related attributes (reset names used
in this module such as snapshoturl2 and the camera2 transform/selection fields)
to safe defaults (None or empty) before returning.
- Around line 619-620: The shell command construction for cmd concatenates
unescaped snapshot_url, self.temp_dir and framefile (e.g., where cmd is built)
which can allow shell injection; fix by avoiding string concatenation and either
(preferably) call wget via subprocess with an argument list (passing
snapshot_url and the output path separately) or properly shell-escape/quote
snapshot_url, self.temp_dir and framefile before building the command; update
the code that constructs cmd to use subprocess.run([...]) or a robust escaping
helper so special characters like & do not get interpreted by the shell.
In `@docs/configuration.md`:
- Around line 114-118: The example shows an incorrect frame range and missing
surrounding blank lines: when camera_switch_interval: 50 the third range should
be "Frames 101-150" (not 101-151) to keep ranges of 50 frames, and the Markdown
table must be separated by blank lines above and below to satisfy MD058; update
the table row "Frames 101-150" and add an empty line before the table and one
after it near the existing "Frames 1-50 | Frames 51-100 | Frames 101-151" block.
In `@klipper_macro/timelapse.cfg`:
- Line 274: The macro default variable variable_camera_switch_interval is set to
50 but needs to match the Python backend default of 5; update the value of
variable_camera_switch_interval in timelapse.cfg from 50 to 5 so the macro and
backend defaults are aligned and avoid inconsistent behavior before settings
sync.
- Around line 197-199: The template currently skips SET_GCODE_VARIABLE when
params.DUAL_CAMERA_NAME is an empty string because it tests truthiness; change
the condition to test definition so empty string is allowed: replace the
conditional "{% if params.DUAL_CAMERA_NAME %}" with "{% if
params.DUAL_CAMERA_NAME is defined %}" (leave the SET_GCODE_VARIABLE call
MACRO=TIMELAPSE_TAKE_FRAME VARIABLE=dual_camera_name
VALUE="'{params.DUAL_CAMERA_NAME}'" unchanged) so the dual_camera_name variable
is updated even when params.DUAL_CAMERA_NAME is empty.
---
Outside diff comments:
In `@component/timelapse.py`:
- Around line 416-423: The settings list named settingsWithGcodechange used to
mark which POSTed settings should trigger gcodechange currently omits the
dual-camera related keys; add 'dual_camera', 'camera2', and
'camera_switch_interval' to settingsWithGcodechange so updates to those settings
set gcodechange=True and macro variables are refreshed—locate the
settingsWithGcodechange definition and append these three keys to the array.
- Around line 500-517: The DUAL_CAMERA_NAME value coming from
self.config['camera2'] must be escaped and quoted to avoid breaking Klipper
shlex parsing; create a sanitized camera2_name by converting
self.config['camera2'] to a string, replacing any internal single-quote
characters with an escaped version, then use that sanitized camera2_name when
building gcommand so the DUAL_CAMERA_NAME parameter is wrapped in single quotes
(update the gcommand construction where DUAL_CAMERA_NAME is appended to
reference the sanitized camera2_name instead of raw self.config['camera2']).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
klipper_macro/timelapse.cfg (1)
272-274:⚠️ Potential issue | 🟡 MinorAlign macro default switch interval with backend default.
variable_camera_switch_intervalis set to50, but the Python component default is5. This mismatch can cause inconsistent behavior until settings sync runs.🔧 Proposed fix
variable_dual_camera: False variable_dual_camera_name: "" -variable_camera_switch_interval: 50 +variable_camera_switch_interval: 5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@klipper_macro/timelapse.cfg` around lines 272 - 274, Update the default switch interval in the timelapse macro so it matches the backend Python component: change variable_camera_switch_interval from 50 to 5 in the timelapse.cfg defaults (alongside variable_dual_camera and variable_dual_camera_name) to ensure consistent behavior until settings sync runs.component/timelapse.py (1)
311-319:⚠️ Potential issue | 🟠 MajorReset camera2 fields when camera is not found.
When
camera2is not found in the webcam config, the code only logs a warning but doesn't reset the camera2 fields. This could leave stale values from a prior configuration.🧹 Proposed fix
else: + self.config['snapshoturl2'] = None + self.config['flip_x2'] = False + self.config['flip_y2'] = False + self.config['rotation2'] = 0 logging.warning( f"timelapse: camera2 '{webcam_name}' not found in " f"webcam config. Available: {list(cams.keys())}" ) except Exception as e: + self.config['snapshoturl2'] = None + self.config['flip_x2'] = False + self.config['flip_y2'] = False + self.config['rotation2'] = 0 logging.info( f"timelapse: error getting camera2 config: {e}" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@component/timelapse.py` around lines 311 - 319, The warning branch that runs when camera2 (referenced as camera2 and webcam_name with cams) is not found should also clear/reset all camera2-related state to avoid stale values; update the else block that currently logs logging.warning(...) to explicitly reset the camera2 fields (e.g., set camera2 and any camera2_* attributes on the surrounding object or config to None/empty/defaults) and do the same in the except Exception as e branch where you log the error so both "not found" and error paths leave camera2 cleared.
🧹 Nitpick comments (1)
component/timelapse.py (1)
522-524: Consider adding a comment explaining the escaping logic.The single-quote escaping pattern using
chr(39)andchr(92)is correct but cryptic. A brief comment would help maintainability.📝 Suggested improvement
+ f" FW_RETRACT={self.config['fw_retract']}" \ + f" DUAL_CAMERA={self.config['dual_camera']}" \ + # Escape single quotes for shell: ' -> '\'' + f" DUAL_CAMERA_NAME='{str(self.config['camera2']).replace(chr(39), chr(39) + chr(92) + chr(39) + chr(39))}'" \ + f" CAMERA_SWITCH_INTERVAL={self.config['camera_switch_interval']}"Or use a more readable approach:
camera2_escaped = self.config['camera2'].replace("'", "'\\''")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@component/timelapse.py` around lines 522 - 524, The DUAL_CAMERA_NAME string uses a cryptic escaping expression (chr(39) and chr(92)) inside the f-string that makes maintenance hard; update the code around DUAL_CAMERA_NAME to either (a) add a concise inline comment explaining that chr(39) is "'" and chr(92) is "\" and why the replace is required, or (b) refactor to compute a clearer temporary variable (e.g., camera2_escaped = self.config['camera2'].replace("'", "'\\''") ) and then use that variable in the f-string; target the expression that references self.config['camera2'] and the DUAL_CAMERA_NAME construction and keep CAMERA_SWITCH_INTERVAL and DUAL_CAMERA references unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@component/timelapse.py`:
- Around line 380-383: The ffmpeg command string currently interpolates raw
paths (filepath, tmpfile, and self.ffmpeg_binary_path) which can break if
self.temp_dir or framefile contain shell metacharacters; wrap these path
components using shlex.quote before building cmd (use
shlex.quote(self.ffmpeg_binary_path), shlex.quote(filepath),
shlex.quote(tmpfile)) so the command is safely quoted; update the lines that
construct filepath/tmpfile and the cmd variable in timelapse.py accordingly and
ensure shlex is imported.
In `@docs/configuration.md`:
- Around line 277-279: The example config in documentation has an inconsistent
default for camera_switch_interval; update the commented example in
docs/configuration.md (the lines with `#dual_camera`, `#camera2`,
`#camera_switch_interval`) to use the same default value as the Python component
(camera_switch_interval = 5 in timelapse.py) so the docs and timelapse.py agree.
---
Duplicate comments:
In `@component/timelapse.py`:
- Around line 311-319: The warning branch that runs when camera2 (referenced as
camera2 and webcam_name with cams) is not found should also clear/reset all
camera2-related state to avoid stale values; update the else block that
currently logs logging.warning(...) to explicitly reset the camera2 fields
(e.g., set camera2 and any camera2_* attributes on the surrounding object or
config to None/empty/defaults) and do the same in the except Exception as e
branch where you log the error so both "not found" and error paths leave camera2
cleared.
In `@klipper_macro/timelapse.cfg`:
- Around line 272-274: Update the default switch interval in the timelapse macro
so it matches the backend Python component: change
variable_camera_switch_interval from 50 to 5 in the timelapse.cfg defaults
(alongside variable_dual_camera and variable_dual_camera_name) to ensure
consistent behavior until settings sync runs.
---
Nitpick comments:
In `@component/timelapse.py`:
- Around line 522-524: The DUAL_CAMERA_NAME string uses a cryptic escaping
expression (chr(39) and chr(92)) inside the f-string that makes maintenance
hard; update the code around DUAL_CAMERA_NAME to either (a) add a concise inline
comment explaining that chr(39) is "'" and chr(92) is "\" and why the replace is
required, or (b) refactor to compute a clearer temporary variable (e.g.,
camera2_escaped = self.config['camera2'].replace("'", "'\\''") ) and then use
that variable in the f-string; target the expression that references
self.config['camera2'] and the DUAL_CAMERA_NAME construction and keep
CAMERA_SWITCH_INTERVAL and DUAL_CAMERA references unchanged.
Comment on lines
+380
to
+383
| filepath = self.temp_dir + framefile | ||
| tmpfile = filepath + ".tmp.jpg" | ||
| cmd = (f"{self.ffmpeg_binary_path} -i '{filepath}'" | ||
| f" -vf '{filter_str}' '{tmpfile}' -y") |
There was a problem hiding this comment.
Use shlex.quote for paths in ffmpeg command.
For consistency with the wget command fix (line 626-627) and to prevent issues if temp_dir contains shell metacharacters, use shlex.quote for the paths.
🔒 Proposed fix
filepath = self.temp_dir + framefile
tmpfile = filepath + ".tmp.jpg"
- cmd = (f"{self.ffmpeg_binary_path} -i '{filepath}'"
- f" -vf '{filter_str}' '{tmpfile}' -y")
+ cmd = (f"{self.ffmpeg_binary_path} -i {shlex.quote(filepath)}"
+ f" -vf {shlex.quote(filter_str)} {shlex.quote(tmpfile)} -y")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@component/timelapse.py` around lines 380 - 383, The ffmpeg command string
currently interpolates raw paths (filepath, tmpfile, and
self.ffmpeg_binary_path) which can break if self.temp_dir or framefile contain
shell metacharacters; wrap these path components using shlex.quote before
building cmd (use shlex.quote(self.ffmpeg_binary_path), shlex.quote(filepath),
shlex.quote(tmpfile)) so the command is safely quoted; update the lines that
construct filepath/tmpfile and the cmd variable in timelapse.py accordingly and
ensure shlex is imported.
Comment on lines
+277
to
+279
| #dual_camera: False | ||
| #camera2: | ||
| #camera_switch_interval: 50 |
There was a problem hiding this comment.
Inconsistent default value for camera_switch_interval.
Line 107 documents the default as 5, and the Python component also uses 5 as the default (line 109 in timelapse.py). However, the example config here shows 50.
📝 Proposed fix
`#dual_camera`: False
`#camera2`:
-#camera_switch_interval: 50
+#camera_switch_interval: 5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/configuration.md` around lines 277 - 279, The example config in
documentation has an inconsistent default for camera_switch_interval; update the
commented example in docs/configuration.md (the lines with `#dual_camera`,
`#camera2`, `#camera_switch_interval`) to use the same default value as the Python
component (camera_switch_interval = 5 in timelapse.py) so the docs and
timelapse.py agree.
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
1 participant
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.
This allows users to create a timelpase with 2 cameras it allows you to set an interval and flick between cameras.
Example -
https://www.youtube.com/watch?v=lsDNlHNkArc
If this is approved I will work in integrating these features into the timelapse settings UI in Mainsail.