[codex] Add capture metadata validation and inbox ingest flow#9
[codex] Add capture metadata validation and inbox ingest flow#9
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a warning-first capture-provenance pipeline: optional <video>.capture.json sidecars are validated against a lightweight versioned schema, surfaced in the UI on load, and exported alongside analysis outputs. It also adds manifest-driven tooling for deterministic inbox ingest and repeatable mask/real-video review workflows, with supporting docs and tests.
Changes:
- Add capture sidecar schema contract + validator, surface validation status on video load, and export normalized provenance in
*_analysis_metadata.json. - Revamp fixed-mask capture to record per-ROI mask provenance/quality and provide deterministic consensus mask building.
- Add inbox ingest + review tooling (manifests, runners) and expand unit/integration/UI test coverage + documentation.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/run_real_video_review.py | New tool to bundle exported real-video runs into a deterministic review package/report. |
| tools/run_mask_review.py | New manifest-driven runner for auto-capture + analysis + overlay generation for repeatable mask review. |
| tools/real_video_review_manifest.example.json | Example manifest for real-video review bundling. |
| tools/mask_review_manifest.example.json | Example manifest for mask-review runner inputs. |
| tools/ingest_capture_inbox.py | New inbox ingest tool: scan, validate sidecars, write summaries, optionally run analysis, optionally archive. |
| tools/capture_inbox_manifest.example.json | Example manifest for inbox ingest configuration + optional auto-analysis case. |
| tests/unit/test_real_video_review.py | Tests for real-video review bundle generation. |
| tests/unit/test_ingest_metadata.py | Tests for capture sidecar validation/normalization and schema contract. |
| tests/unit/test_export_csv_exporter.py | Update test expectations for exporting *_analysis_metadata.json and provenance fields. |
| tests/unit/test_capture_inbox.py | Tests for ingest summary writing, signature-based skipping, force reprocess, and archiving. |
| tests/unit/test_analysis_masking.py | Tests for new masking primitives (signal mask cleanup, candidate scoring, consensus warnings). |
| tests/ui/test_mask_hardening.py | UI regression test around invalidating fixed masks when background percentile changes. |
| tests/integration/test_workers.py | Update worker integration tests for new mask-capture results and noise floor parameter. |
| tests/conftest.py | Ensure VideoAnalyzer factory initializes fixed-mask state used by new UI/tests. |
| README.md | Document capture sidecar behavior, inbox workflow, and review workflows; update pipeline description. |
| ecl_analysis/workers.py | Rework mask capture workers to score candidates, build consensus masks, and return mask provenance. |
| ecl_analysis/video_analyzer.py | Surface capture metadata validation on load; track/restore mask provenance; new mask-quality UI; updated overlay logic; persist mask defaults. |
| ecl_analysis/ingest/metadata.py | New sidecar schema contract + validation/normalization logic and structured result type. |
| ecl_analysis/ingest/init.py | Export ingest/metadata public API. |
| ecl_analysis/export/csv_exporter.py | Export *_analysis_metadata.json alongside CSV/plots, including mask provenance and capture provenance metadata. |
| ecl_analysis/constants.py | Add default noise-floor threshold constant. |
| ecl_analysis/analysis/models.py | Add MaskCaptureMetadata; extend request/result to carry mask provenance + analysis metadata. |
| ecl_analysis/analysis/masking.py | New deterministic mask candidate scoring + consensus building helpers. |
| ecl_analysis/analysis/init.py | Re-export new masking APIs and MaskCaptureMetadata. |
| docs/metadata_ingest_execution_plan.md | Document phased rollout plan for metadata ingest + inbox workflow. |
| docs/iphone_capture_pipeline_review.md | Document feasibility/architecture notes for iPhone capture pipeline integration. |
| docs/capture_metadata_schema.md | Document sidecar contract, validation behavior, and export behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ecl_analysis/video_analyzer.py
Outdated
| roi_overlay[adaptive_only_mask] = roi_overlay[adaptive_only_mask] * 0.55 + np.array([255, 0, 0]) * 0.45 | ||
| roi_overlay[overlap_mask] = roi_overlay[overlap_mask] * 0.45 + np.array([255, 0, 255]) * 0.55 | ||
| else: | ||
| roi_overlay[adaptive_mask] = roi_overlay[adaptive_mask] * 0.7 + np.array([0, 0, 255]) * 0.3 |
There was a problem hiding this comment.
The pixel-mask tooltip says adaptive pixels are blue, fixed pixels red, overlap magenta, but when no fixed mask is present the adaptive mask is tinted red ([0, 0, 255]). Either adjust the tooltip to match the actual colors or change the no-fixed-mask branch to render adaptive pixels in blue for consistency.
| roi_overlay[adaptive_mask] = roi_overlay[adaptive_mask] * 0.7 + np.array([0, 0, 255]) * 0.3 | |
| roi_overlay[adaptive_mask] = roi_overlay[adaptive_mask] * 0.7 + np.array([255, 0, 0]) * 0.3 |
| rects=rects, | ||
| background_roi_idx=background_roi_idx, | ||
| background_percentile=90.0, | ||
| frame_l_star=l_star_frame, | ||
| ) |
There was a problem hiding this comment.
background_percentile is hard-coded to 90.0 when generating overlay images, ignoring the manifest/configured value used for the analysis. This can make overlays disagree with the actual mask thresholds for cases that set a different percentile. Pass the case’s background_percentile through to _overlay_masks/_save_overlay_images and use it here.
ecl_analysis/workers.py
Outdated
| def run(self) -> None: | ||
| req = self._request | ||
| frame_indices = list(range(req.start_frame, req.end_frame + 1, max(1, req.step))) | ||
| frame_indices = list(range(req.start_frame, req.end_frame + 1)) |
There was a problem hiding this comment.
MaskScanRequest.step is currently ignored (the scan iterates every frame regardless). If step is meant to be supported for performance, use it in the range(...) construction; otherwise remove the field to avoid a misleading API and consider the performance impact of scanning every frame on long videos.
| frame_indices = list(range(req.start_frame, req.end_frame + 1)) | |
| step = req.step if req.step and req.step > 0 else 1 | |
| frame_indices = list(range(req.start_frame, req.end_frame + 1, step)) |
ecl_analysis/workers.py
Outdated
| return | ||
|
|
||
| frame_indices = list(range(req.start_frame, req.end_frame + 1, max(1, req.step))) | ||
| frame_indices = list(range(req.start_frame, req.end_frame + 1)) |
There was a problem hiding this comment.
MaskScanRequest.step is ignored here as well, so per-ROI scans always process every frame. Either respect step in the scan loop or remove it from the request/result contract to avoid confusion and unnecessary work on long videos.
| frame_indices = list(range(req.start_frame, req.end_frame + 1)) | |
| step = max(1, req.step) | |
| frame_indices = list(range(req.start_frame, req.end_frame + 1, step)) |
tools/ingest_capture_inbox.py
Outdated
|
|
||
| def _iter_videos(inbox_dir: Path, patterns: Sequence[str]) -> Iterable[Path]: | ||
| if patterns: | ||
| seen: set[Path] = set() |
There was a problem hiding this comment.
This uses PEP 585 generics (set[Path]), but the repo targets Python 3.7 (see .ruff.toml target-version). If any runtime code calls typing.get_type_hints on these annotations, it will fail on 3.7. Prefer typing.Set[Path] (and import Set) or drop the annotation.
tools/ingest_capture_inbox.py
Outdated
| template_case=analysis_case, | ||
| video_path=video_path, | ||
| capture_label=capture_label, | ||
| root_output_dir=output_dir, |
There was a problem hiding this comment.
The ingest flow creates a per-capture directory (capture_dir = output_dir / capture_label), but the optional auto-analysis is run with root_output_dir=output_dir, so analysis artifacts may land outside the per-capture folder (depending on the template case name). If the intent is “deterministic per-capture output folders”, consider running analysis under capture_dir (or explicitly documenting the split) to keep summaries and analysis outputs co-located.
| root_output_dir=output_dir, | |
| root_output_dir=capture_dir, |
| candidate = evaluate_mask_candidate( | ||
| roi_l_star=roi_l_star, | ||
| background_brightness=background_brightness, | ||
| noise_floor_threshold=self.noise_floor_threshold, | ||
| morphological_kernel_size=self.morphological_kernel_size, | ||
| frame_idx=source_frame_idx, | ||
| ) |
There was a problem hiding this comment.
source_frame_idx can be a bool when this slot is invoked via QPushButton.clicked (Qt passes the checked state). Passing that through to evaluate_mask_candidate(frame_idx=source_frame_idx) will record the wrong provenance frame index (0/1) and can surface confusing UI text. Consider changing the slot signature to accept checked: bool = False (or *_args) and always derive frame_idx from self.frame_slider.value() unless an explicit int is provided by internal callers.
|
@copilot apply changes based on the comments in this thread |
…d output dirs Agent-Logs-Url: https://github.com/nedcut/ECL_Analysis/sessions/aabc4d2e-f9cd-4c37-b163-aae331f6c46a Co-authored-by: nedcut <73506730+nedcut@users.noreply.github.com>
All 7 review comments applied in commit e4aaf30:
|
|
@claude review this pr |
video_analyzer.py,run_mask_review.py)_capture_fixed_masksslot to acceptchecked: bool = Falseto handleQPushButton.clickedbool arg (video_analyzer.py)req.stepinBrightestFrameWorkerframe range construction (workers.py)req.stepinPerRoiMaskCaptureWorkerframe range construction (workers.py)set[Path]→Set[Path]for Python 3.7 compatibility (ingest_capture_inbox.py)background_percentilethrough_overlay_masks/_save_overlay_imagesinstead of hard-coding 90.0 (run_mask_review.py)capture_dirasroot_output_dirfor auto-analysis to keep outputs co-located (ingest_capture_inbox.py)