Feature: encoding, UI and performance improvements - #114
Open
C-Achard wants to merge 188 commits into
Open
Conversation
Extract the RecorderStats dataclass from dlclivegui/services/video_recorder.py into dlclivegui/utils/stats.py and add the needed dataclasses import. Update imports to reflect the new location in dlclivegui/gui/recording_manager.py, dlclivegui/services/video_recorder.py, and tests/tests/gui/test_rec_manager.py. Also add imports for REC_DO_LOG_TIMING and WorkerTimingStats in video_recorder.py. No behavioral changes intended—this is a refactor to centralize recorder-related stats.
Basler backend: add preserve_mono support and configure the pypylon converter to output Mono8 when the camera source PixelFormat is Mono* and preserve_mono is enabled; fall back to BGR8 otherwise. Log the first decoded frame and expose per-backend timing flag BASLER_DO_LOG_TIMING. Update defaults for global timing flags and add REC_DO_LOG_TIMING. Camera settings: add preserve_mono flag and include it in __repr__. RecordingManager: pass convert_grayscale_to_rgb based on the camera preserve_mono setting. VideoRecorder: add convert_grayscale_to_rgb option, avoid unnecessary grayscale->RGB expansion when disabled, forward pixel-format/size hints to WriteGear, add WorkerTimingStats for recorder processing and writer, instrument preprocessing/queue/write steps, log the first frame, and improve frame-size mismatch handling and error reporting. These changes reduce memory/CPU overhead for mono cameras and add better timing/diagnostics for recording.
Introduce a preserve_mono capability and related properties to CameraBackend (actual_pixel_format, recommended_preserve_mono). Add a Preserve Mono checkbox to the camera config UI, persist/load its value, include it in probe detection logic (detect pixel format and apply recommended preserve_mono when supported), and treat changes to preserve_mono as restart-triggering. Update Basler static capabilities test to advertise preserve_mono support and add VideoRecorder tests to verify grayscale frames are preserved when requested and expanded by default. This enables preserving single-channel camera output to reduce bandwidth/overhead for monochrome cameras.
Expose the camera source pixel format via actual_pixel_format and add recommended_preserve_mono to suggest preserving mono images when the source format starts with "Mono". Add "preserve_mono" to reported capability levels. Implement _read_source_pixel_format to centralize reading the PixelFormat feature and call it from _configure_converter when needed so the backend always knows the source format before configuring conversion.
Rename internal source pixel format to _camera_pixel_format and centralize pixel-format handling. Add actual_pixel_format and actual_output_format properties, plus helpers (_read_camera_pixel_format, _is_camera_mono, _should_output_mono) to determine if the camera is mono and whether the backend should output mono frames. Update _configure_converter to use these helpers (emit Mono8 when preserve_mono + mono camera), remove the _output_is_mono flag and the old _read_source_pixel_format, and improve log messages. Also ensure the camera pixel format is read during startup after gain detection.
Add UI and backend support for reporting the camera backend's detected output format and pixel format. Introduce a detected output label with tooltip and move/rename the "preserve mono" checkbox into an Output row. Store/clear detected_output_format and detected_pixel_format in camera props, read actual_output_format from backends during probing, and set detected_output_format when pixel format indicates Mono. Add a mono indicator to camera list entries and update probe early-return logic to require both resolution and output format before skipping probing.
Expose actual_pixel_format and actual_output_format across backends and track the camera-reported formats for UI/telemetry. Aravis: add actual_pixel_format/actual_output_format properties and record _camera_pixel_format when setting pixel format. GenTL: initialize _camera_pixel_format/_actual_output_format, add _output_format_for_frame to infer output format from numpy frames, populate _actual_output_format on read, and record detected camera pixel format in several places. OpenCV: add actual_pixel_format (None) and actual_output_format (BGR8). These changes provide a consistent way to report native and emitted pixel formats to callers.
Replace the manual QWidget/QHBoxLayout construction for the "Output" settings row with the reusable make_two_field_row helper. This simplifies and standardizes the layout while preserving the same widgets (cam_preserve_mono_checkbox and detected_output_format_label) and applies key_width=60 and gap=40 before adding the row to dlg.settings_form.
Add human-friendly display IDs for multi-camera support. Introduce get_display_id(settings) which prefers settings.name, then properties[backend].device_name, and falls back to backend:index. Main window now stores per-camera display IDs, clears them on stop, passes labels to create_tiled_frame, and uses the display label when building the compact camera status lines. This separates internal camera IDs from user-facing labels for clearer UI.
Adjust tests to reflect new human-friendly display_id values and add a fallback case. Updated expectations in tests to assert display_id equals "GenTL cam" / "GenTL Cam" / "C1" where applicable, and added a unit test to verify get_display_id falls back to the backend index (e.g. "gentl:3") when camera name is empty. Also added assertions in the controller test to ensure the stable camera id is present in frames and correctly mapped to the display id. Files changed: tests/gui/test_rec_manager.py, tests/services/test_multicam_controller.py.
Delete the internal _create_tiled_frame implementation and the public frame accessors (get_frame, get_all_frames, get_tiled_frame) from MultiCameraController. This removes the tiled canvas construction logic and convenience getters for retrieving camera frames; update any callers to use the controller's new/alternate APIs or access frames via the updated code paths.
Collect and propagate runtime camera info to improve recorder FPS selection and logging. - Disabled some verbose timing flags in config (REC_DO_LOG_TIMING, BASLER_DO_LOG_TIMING). - MultiCameraController: added runtime_info signal, stores per-camera runtime info, logs it, and exposes actual_fps_by_camera_id(). Workers emit backend runtime properties on open. - MainWindow: pass actual_fps_by_camera to RecordingManager when starting recordings. - RecordingManager: added backend namespace helper and _resolve_recording_fps(cam, cam_id, frame_rates) to prefer measured FPS, then backend-detected FPS, then requested cam.fps (or auto). Use resolved recorder_fps when creating VideoRecorder and log chosen values. - VideoRecorder: if frame_rate is missing/zero, fall back to 30 FPS and emit a warning; added startup info log; removed/commented the old pix_fmt/output_dimensions branch. These changes make recording frame rates more accurate by preferring runtime-measured FPS and provide clearer logging and safe fallbacks when FPS is unknown.
Fix the video encoding error path in VideoRecorder by replacing _process_timing with _writer_timing so encoding failures are recorded and logged against the correct timing object. This ensures error timing and maybe_log are invoked on the writer timing tracker rather than the wrong object.
Add _camera_pixel_format and _actual_output_format attributes to the Aravis backend and update the GenTL backend to set _camera_pixel_format (replacing the previous _actual_pixel_format assignment). This unifies pixel-format state handling across camera backends and prepares for explicit output format tracking.
Use the normalized backend value and integer camera index when building display IDs, ensuring consistent fallback identifiers. Also remove incorrectly rebased `_recording_frame_emission_enabled` as it is no longer needed.
Ensure per-camera runtime metadata is fully reset across lifecycle transitions in `MultiCameraController`. This clears `_runtime_info` when starting a new camera set, removes entries when an individual camera is torn down, and clears all runtime info during final stop cleanup. It also removes the obsolete FIXME/commented-out clear call.
Initialize FPS with a safe default, parse `frame_rate` defensively, and only warn/fallback when the configured value is missing or non-positive. This avoids startup failures from invalid frame-rate values while preserving expected logging. The commit also removes stale commented-out writer configuration code.
Camera probing no longer silently turns on `preserve_mono` when a mono pixel format is detected. Instead, it records `recommended_preserve_mono` and shows a status message explaining why enabling “Preserve mono frames” is recommended for performance, keeping configuration changes explicit and user-controlled.
Refactors `test_preview_renders_frames` to remove real camera timing dependencies by monkeypatching controller lifecycle methods and emitting synthetic `MultiFrameData` via `QTimer`. The test now explicitly verifies preview start/stop state changes, frame assignment, and that the displayed pixmap actually updates (via cache key), making the functional GUI test faster and less flaky.
Centralize WriteGear option building and validation in a shared utility, including strict checks for FPS/codec/CRF and fallback to a default recording FPS. Recording settings now support `fast_encoding`, which injects ultrafast low-latency FFmpeg overrides for x264/x265, and the recording manager passes these overrides through to `VideoRecorder`.
Documents that camera config workers only prepare settings and emit requests, while actual backend open/probe/close are handled in the GUI thread to avoid concurrent device opens. This renames `CameraProbeWorker.success` to `probe_requested`, updates the dialog signal hookup, refreshes worker messaging/docs, and removes unused backend cleanup logic from `CameraLoadWorker`.
Refactor `RecordingManager._resolve_recording_fps` to use a new `_valid_fps` helper that accepts only finite, positive numeric values. This removes broad exception handling, centralizes FPS validation for measured/detected/requested sources, and correctly treats invalid, missing, `NaN`, or infinite values as unset.
Update `normalize_writegear_options` to drop keys with `None` values before normalizing known fields. This prevents unset overrides from being propagated as invalid WriteGear/ffmpeg options while preserving explicitly provided settings.
Move final option validation responsibility into `normalize_writegear_options` and simplify `build_writegear_options` to only assemble defaults. `build_writegear_options` now keeps `crf` as provided, defaults codec inline, and only applies override entries with non-`None` values. The normalizer now works from a direct copy of the final options and performs normalization/validation there.
Switch Basler camera startup to `pylon.GrabStrategy_OneByOne` instead of `LatestImageOnly`, and update the nearby identity-persistence comment for clarity.
Add ignore patterns for generated profiling files (`profile*.svg`, `scalene*.json`, and `scalene*.html`) so local performance analysis outputs are not accidentally committed.
Replaced the service’s direct dependency on `BaseProcessorSocket.do_build_in_worker` with `processor_builds_in_worker` from `processor_utils`, improving reuse and reducing coupling to socket internals. Also updated the recording context docstring to use `timestamp_json_files` for consistent key naming.
Store the processor recording context when recording starts and reuse that same context when notifying `on_recording_stopped`, instead of rebuilding it from recorder state. This also centralizes stop-time processor finalization (stop hook + save) and invokes it both on normal async stop and during shutdown when a recording context exists.
Adjust shutdown/reset flow to avoid cleaning up the processor while the worker thread is still alive. Cleanup now runs immediately only after a successful stop, and is deferred to the reaper path when a pending reset completes after the worker eventually joins.
Corrects the recording-stop cleanup path to clear `_processor_recording_context` instead of an unrelated attribute. This ensures the async stop handler sees the finalized state consistently and avoids stale recording context after shutdown.
Refines recording lifecycle integration by capturing finalized recording file context when notifying `on_recording_stopped`, and introducing tracking for recording-start notifications during DLC initialization. It also tightens processor teardown behavior by distinguishing pending reset vs pending plugin cleanup, and ensures full processor plugin cleanup when stopping DLC from the UI/service.
Improve processor metadata finalization by rebuilding recording context from the current run directory and then overlaying cached values, so missing fields are backfilled before merging file context. Also update socket processor saving so explicitly provided relative filenames are saved under the `data/` directory, matching expected output behavior.
Adds focused tests around processor recording context handling in `DLCLiveMainWindow`, including file metadata propagation, stop-time context refresh, and optional processor hooks. It also introduces a regression test for `BaseProcessorSocket.save()` to ensure explicit relative filenames still write under the legacy `data/` directory. UI label testing for unknown camera IDs was relaxed to assert the neutral text is present rather than requiring an exact string.
Set `_processor_recording_started_notified` immediately after a successful recording-start hook callback so the UI tracks notification state correctly and avoids repeated notifications. Also add a unit test that verifies `ProcessorSpec` processors are instantiated on the `DLCLiveWorker` thread, wired into `DLCLive`, and properly stopped/cleared when resetting the processor.
… during recording When the user clicks "Stop pose inference" before "Stop recording", the processor instance was destroyed by reset() without saving its accumulated data. Later the recording stop flow would find no processor instance and silently skip the save. Now _stop_inference() saves processor data first if recording is still active, so data is preserved regardless of stop-button order.
`shutdown()` was skipping `_cleanup_processor()` when the worker thread stopped cleanly, leaving the custom processor's resources unreleased and its buffered data unsaved. This commit adds the missing `_cleanup_processor()` call before tearing down the DLCLive instance.
Stopping the DLC processor during a recording skips the processor's `on_recording_stopped` hook, which would normally handle legacy output copies and DB-compatible file alignment. Show a confirmation dialog when the user attempts to stop inference while recording is still active, recommending they stop recording first.
Co-authored-by: Cyril Achard <cyril.achard@epfl.ch>
Co-authored-by: Cyril Achard <cyril.achard@epfl.ch>
C-Achard
added
enhancement
C-Achard
force-pushed
the
cy/feature-perf-improvement
branch
from
August 20, 2026 10:17
73a61a4 to
c9264f3
Compare
deruyter92
reviewed
Aug 20, 2026
deruyter92
left a comment
deruyter92
left a comment
Contributor
There was a problem hiding this comment.
Great PR, with a lot of changes. I think they are definitely an improvement. great work!
I think good to release as soon as possible, which means we can use the improvements and further test it.
At the same time, as discussed, let's keep in mind that we need to thoroughly test this in practice as well. We need to keep alert about the saving logic; edge-cases that leave the gui unresponsive at the wrong moment; or any risk of data corruption. Together with the planned issues it might be worthwile to organize a test session in practice again. These planned refactors might help as a double review round to surface difficult-to-spot issues.
For now, I found 3 potential issues that I wanted to flag, but for the rest looks very good.
| if recording_enabled and recording_sink is not None: | ||
| try: | ||
| with self._timing.measure("Single.recording_sink"): | ||
| recording_sink(self._camera_id, frame, timestamp, timestamp_metadata) |
Contributor
There was a problem hiding this comment.
this recording_sink appears to enqueue frame by reference. This could be risky if the OpenCV capture backend reuses that buffer on a subsequent grab (the queued frame could be modified before the writer processes it). This seems especially risky after cropping when numpy slices it.
Shal we .copy() here or maybe in the OpenCV read()?
Comment on lines
2484
to
+2493
| def _stop_inference(self, show_message: bool = True) -> None: | ||
| if self._rec_manager.is_active: | ||
| answer = QMessageBox.question( | ||
| self, | ||
| "Stop inference while recording?", | ||
| "This will stop the currently running DLC-live custom processor, if any.\n" | ||
| "File saving will not be handled via the standard 'recording stopped' event hooks." | ||
| "The processor might still save data now, but it will not be paired with the recording.\n\n" | ||
| "Stop inference anyway?", | ||
| ) |
Contributor
There was a problem hiding this comment.
Want to flag here that this currently relies on the processor owning saving logic whenever stop() is invoked. This currenlty is not the case for the example dlc_processor (bare stop does not save).
Comment on lines
+1911
to
+1913
| except Exception as e: | ||
| logger.exception("Error while stopping recording: %s", e) | ||
| return |
Contributor
There was a problem hiding this comment.
Do we want to handle this case more explicitly?
We now return without emitting _recording_stopped_async, so _recording_stopping stays True and both record buttons stay disabled.
The user cannot click Stop again, and nothing is actually retrying the stop, so the stop is not still running. The only way to exit is restarting, and only a log line and no UI error is emitted.
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
2 participants
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.
Scope
Grouped PR containing the latest round of improvements.
Warning
For a detailed account of all features see Contents below.
Contents