Multi camera support#12
Open
alexevag wants to merge 16 commits into
Open
Conversation
Interface previously held a single self.camera. Replace with self.cameras
dict keyed by f"{video_aim}_{camera_idx}", so two cameras can coexist in
the same setup (e.g. one openfield cam feeding DLC plus one passive eye
cam recording to disk). Always including camera_idx in the key keeps the
mapping "physical camera -> file" stable across config edits.
- Interface._initialize_camera loops over all SetupConfiguration.Camera
rows for the setup_conf_idx, builds Camera instances with per-camera
filenames (f"{animal}_{session}_{aim}_{idx}").
- Interface.release iterates over self.cameras.
- WebCam and PiCamera now accept camera_num (defaults to 0) and use it
for cv2.VideoCapture / Picamera2 device selection instead of the
hard-coded 0.
- Camera.__init__ derives the h5 timestamp filename from self.filename
(which carries the per-camera disambiguator), so two cameras in the
same session no longer overwrite each other's h5.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The previous release() guarded each stop_rec call with `if cam.recording.is_set()`. With multi-camera setups that race fires: between mp.Process.start() and the child reaching `recording.set()`, a spawned-but-not-yet-recording subprocess holds resources (open file handles, FFmpegWriter, possibly a /dev/videoN FD) but is_set() returns False, so stop_rec is skipped and the subprocess leaks. - Camera.stop_rec is now idempotent: returns early if camera_process is None, swallows the rare ValueError from close() racing with terminate, joins twice (timeout=30 + timeout=5 after terminate), and clears self.camera_process so a second call is a no-op. - Interface.release calls stop_rec unconditionally — the stop event is sufficient to signal shutdown regardless of subprocess lifecycle step. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two review-flagged issues, both now visible because of multi-camera:
1. WebCam.__init__ previously opened /dev/videoN via cv2.VideoCapture
in the parent, released it, then forked a child that re-opened the
same device. V4L2 drivers don't always tear down state synchronously
on close(), so two cameras initialized back-to-back could race —
second child opens before kernel finishes cleaning up first parent.
Replaced with os.path.exists("/dev/videoN") — race-free, still
catches the common config errors (missing device, wrong camera_num).
Permission / device-busy errors surface from the child's open in
recording_init() as before.
2. The h5 timestamp Recording row was logged with source_path/
target_path pointing at the video directory, but the file actually
lives under logger.source_path + "Recordings/<animal>_<session>/"
(per Logger.createDataset). With per-camera h5 filenames now in
play, the mismatch is more impactful — downstream consumers that
resolve h5 paths via Recording rows would look in the wrong place.
Mirror createDataset's path logic in Camera.__init__ so the row
matches reality.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
cameras producing multiple source directories to drain: - copy_file: routine "transferred" and "deleted" logs demoted from info to debug (they fire per file; info-level for two cameras with hundreds of frame files each was overwhelming). - clear_local_videos: explicit error log when source/target paths are missing, with an actionable "Create it with: mkdir -p ..." hint; warning if no files were found to transfer; file count visible in the transfer start log; warning if the source folder isn't empty after transfer (indicates a partial failure that previously went silent).
Add stable camera addressing and robustness fixes. - schema: add device_id (varchar) to SetupConfiguration with empty default, allowing cameras to be addressed by /dev/v4l/by-id symlink or a serial substring (falls back to camera index when empty). - WebCam: accept and store device_id - Replace simple /dev/videoN stat check with _resolve_device() that: returns camera_num when device_id is empty, accepts an existing path, or matches a single by-id index0 symlink under /dev/v4l/by-id. Sets self.device before spawning child to avoid race conditions. - Use self.device when opening cv2.VideoCapture and pin capture options (FOURCC=YUYV, CONVERT_RGB, BUFFERSIZE=1) to ensure consistent frames and reduce buffering/staleness. - Ensure captured grayscale frames are cast to uint8 and improve error/warning messages (fix typo, warn when property set() is rejected). These changes improve stable camera selection across reboots/USB re-plugs and make capture initialization more robust and predictable.
Clean up docstrings and comments, reduce noisy prints, and add throttled logging and robustness fixes across interface, Camera, and DLC modules. - src/ethopy/core/interface.py: Simplified and clarified docstrings for camera initialization and release, emphasize stable camera->file mapping and explain unconditional stop_rec call to avoid races. - src/ethopy/interfaces/Camera.py: Clarified comments around camera naming and device resolution; made per-camera timestamp filename behavior explicit; added _last_frame_err_log to throttle per-frame read-error logs; simplified stop_rec and process-close handling; improved device-resolution error messages and camera property comments; ensured consistent behavior for capture format and buffer settings. - src/ethopy/interfaces/dlc.py: Added logging (logger import and module logger); convert frames to float32 before passing to DLCLive; replace prints with structured logging (debug/warning/exception); introduce a _throttled_log helper to avoid spamming logs in tight loops; add debug/warning checks around frame transfer delays, corner detection, and process lifecycle; handle case of no confident corner frames gracefully; use debug for shared memory unlink messages. Overall these changes improve observability, reduce console spam, make DLC inputs type-consistent, and clarify camera/device handling to make startup and shutdown more robust.
Replace the truthiness check 'if not recs' with 'if len(recs) == 0' when computing rec_idx in Logger. This avoids ambiguous truth-value errors for array-like objects (e.g., numpy arrays) and ensures rec_idx is set to 1 for empty collections, otherwise max(recs) + 1.
write the .mp4 into the same Recordings/{animal}_{session}/ folder as the timestamp/DLC h5 (drops the now-unused video_source_path keys), and make clear_local_videos move only this camera's own files without rmdir, so it can't clobber the Writer-owned h5 files or other cameras' videos.
get_table_keys can return a scalar primary key first (e.g. response_port before response_loc_x), so the old core[0] check missed the iterable and inserted tuples into scalar columns. Scan all non-hash primary keys for a list/tuple instead. It used hasattr(..., "__iter__"), which treats strings as expandable, and the inner split used a negative scalar test (int/float/str) that misclassified numpy scalars as sequences and crashed on indexing.
…amps The MJPEG stream ignored server.port: HTTPServerThread was constructed without serve_port, so it always bound 8000 even when a different port was configured. Pass self.serve_port through. WebCam wrote timestamps only via self.dataset, which setup() leaves None on the txt path (logger=None), so a logger-less WebCam crashed with AttributeError in write_frame and at the end of rec(). Guard both o tmst_type, mirroring PiCamera.
The inline expansion in log_conditions mixed three concerns (detect a sequence primary key, validate lengths, split into rows) in a nested loop that was hard to follow. Move it to task_helper_funcs.expand_condition_rows with a docstring explaining the semantics, so the call site is just "build the rows, insert each." The helper also validates that all sequence fields share one length and raises a clear error instead of the previous raw IndexError / silent truncation on mismatched lengths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Unit tests for the condition-row expansion helper: scalar passthrough, single- and parallel-sequence expansion, trigger on any primary key, non-primary sequences left whole, strings/numpy scalars treated as single values, and the unequal-length ValueError. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
test_is_hydrated assigns into behavior.session_params, but the fixture built a Behavior() without calling setup(), leaving session_params as None (its __init__ default). Initialize it to an empty dict in the fixture, matching how the other mocked attributes are set up.
…_rows Add cases for an empty sequence (zero rows) and a tuple secondary field (split element-by-element). Both lock in the existing behavior, which a differential check confirmed is identical to the pre-refactor inline logic; the tuple case also documents that it intentionally differs from factorize.
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.
Description
Refactor camera handling to support multiple cameras per setup and makes video recording share the session's data paths. Also folds in a correctness fix and refactor for condition-row generation that the branch had accumulated(this bug was latent on the old Python/numpy but surfaced once the code was run on newer versions).
Camera:
video_aim+camera_idx, each recording to its own file.Interface.camerabecomesInterface.cameras(a dict), andrelease()tears every camera down.device_idso a camera can be addressed by a stable/dev/v4l/by-ididentifier (survives reboots / re-plugging) instead of an enumeration index; falls back tocamera_numwhen empty. Newdevice_idcolumn onSetupConfiguration.Camera.source_path/target_path(renamed fromvideo_source_path/video_target_path);clear_local_videosmoves only this camera's own files and no longer risks copying a file onto itself.stop_recidempotent and safe to call before startup finishes.server.portto the MJPEG stream (previously hardcoded to 8000) and letWebCamrun without a logger (write timestamps to a txt file).DLC:
print()debugging with throttled logging.Conditions (
log_conditions):Expand a condition into rows when any primary key holds a sequence (not just
the first key), correctly treating strings and numpy scalars as single values.
Validate that all sequence fields share one length and raise a clear error
instead of a raw
IndexError/ silent truncation.Extract the expansion into
task_helper_funcs.expand_condition_rowsforreadability.
Tests:
tests/test_task_helper_funcs.pycoveringexpand_condition_rows: scalar passthrough, single- and parallel-sequence expansion, trigger on any primary key, non-primary sequences left whole, strings/numpy scalars treated as singlevalues, and the unequal-length
ValueError.test_is_hydratedfailure (a test-setup bug: it assignedmax_rewardintoparamswhileis_hydratedreadssession_params, and the fixture leftsession_paramsasNone).Type of change
Checklist: