feat(ui): add Clip Deduplicator and Overlap Manager for lossless clip merging - #27
Victor7996 wants to merge 20 commits into
Conversation
…ioning - Switch AMD AMF hardware encoder session to NV12 Direct3D 11 texture frames with D3D11 Video Processor BT.709 color conversion, resolving vertical striping artifacts on AMD GPUs. - Silence experimental coroutine deprecation warnings in FTHRclips.vcxproj. - Align version statements across version.py, installer_windows.iss, and RELEASE_NOTES.md to 1.1.0-alpha.
…ware-encoding fix: AMF NV12 hardware encoding pipeline and 1.1.0-alpha release versioning
- Extend native replay buffer limits up to 30 minutes (1800s). - Add Clip Deduplicator to scan and merge overlapping clips losslessly. - Add UI warning for high memory requirements on large buffers. - Handle EXPORT_CANCELLED signal gracefully without magic strings. - Add deduplication UI and preview dialog. - Update KNOWN_ISSUES and docstrings regarding Greedy Pairing.
|
Thanks for putting this together — the feature direction looks useful, and I like the separation of the deduplication core from the Qt UI. The FFmpeg argument-list usage, Before merging, I have a few safety concerns:
I think this is a promising PR, but because it changes resource limits and can delete recordings, I would prefer these safety issues to be addressed before approval. |
- fix(win32): use c_void_p + create_unicode_buffer to prevent LPCWSTR null-truncation in SHFileOperationW fallback - feat: add _is_file_locked() guard in quarantine_original_clips to skip files still held open by the capture engine (prevents EBUSY on Linux and silent partial-file moves on Windows) - fix: get_safe_output_path now has a max_attempts=9999 upper bound to eliminate the infinite-loop risk on full/network disks - fix(ui): regression in _on_scan_finished: has_inferred was comparing against the old 'exact' literal; corrected to confidence == 'LOW' - fix(ui): _MergeWorker.finished signal now carries remove_originals as a bool so _on_merge_finished cannot observe a stale checkbox state - fix: calculate_timestamp_confidence uses ctime > mtime + 60 directly, removing the spurious derived_start_time indirection; adds comment explaining Linux vs Windows ctime semantics - fix: remove dead time.sleep(0.05) after raise in copy branch of merge_overlapping_pair (indentation bug / merge artifact) - feat: merge_clip_cluster now returns tuple[Path, int] and calculates actual_saved_bytes before quarantining originals - fix: cluster_overlapping_clips adds defensive sort by start_time - test: replace trivial assert-always-passes confidence test with two meaningful tests (HIGH for fresh file, LOW for backdated mtime) - test: add test_merge_clip_cluster_integration covering A->B->C cluster merge with real FFmpeg, quarantine, and saved-bytes assertion ruff: 0 errors pytest: 18/18 passed
…ngs issues - fix(security): ensure _is_file_locked does not misinterpret read-only files as locked - fix(quarantine): return actually quarantined paths from quarantine_original_clips - fix(manifest): use MANIFEST_SUFFIX (.fthr-audio.json) with fallback for .fthr-manifest - fix(quarantine): prevent overwrite collisions in .fthr_quarantine fallback - fix(toctou): add finalize_output to guarantee atomic safe placement before move - fix(naming): strip existing _merged_# suffix in get_safe_output_path to avoid accumulation - feat(cluster): integrate cluster_overlapping_clips and find_overlapping_clusters in UI - feat(cluster): support Sequence[ClipRecord], directory isolation, and multi-clip chains - fix(savings): accurately calculate disk savings based on successfully quarantined files - fix(ffmpeg): add _run_ffmpeg_tool draining stderr continuously to prevent pipe deadlock - feat(compat): add are_clips_stream_copy_compatible to prevent corrupt merges on mismatched resolutions - fix(ui): default LOW confidence items to unselected in deduplication dialog - fix(ui): support multi-clip preview switching in ClipPreviewDialog - fix(ui): harden _teardown_workers with terminate() fallback to avoid destroyed QThread crash - test: add comprehensive tests for read-only, locked, suffix dedup, clusters, and exception gate
…2 on tempdir cleanup
81de054 to
8267489
Compare
kalopsia-space
left a comment
kalopsia-space
left a comment
There was a problem hiding this comment.
Review
Verdict: Changes requested.
The deduplication safety work is substantially improved: deletion is opt-in, originals are quarantined, output collisions are handled, FFmpeg arguments are passed safely, and the PR contains focused tests.
There is still a significant Linux resource concern. The Linux engine now permits a 30-minute in-memory encoded replay buffer. At the configured maximum of 60,000 kbps, that is approximately 13.5 GB of video data before audio and overhead. The PR does not appear to add an equivalent disk-backed Linux spool or a hard measured memory guard.
Please add a bounded Linux disk-backed path, impose a realistic duration/bitrate memory cap, or reject configurations that exceed a measured budget. Add a native test for the actual resource guard rather than only duration/frame-count arithmetic.
The current head also has failing CI, including Linux CMake and Ubuntu Python jobs. The claimed passing result in the description is not sufficient evidence for the current head. Please fix or explicitly classify the failures and rerun the required matrix. git diff --check also reports whitespace issues, and CI should ensure FFmpeg integration tests run rather than all skipping when FFmpeg is absent.
|
Follow-up clarification: Windows CI failures are not blockers for this PR because it targets the maintained |
…cessive configs, and resolve review findings - Enforce hard byte memory guard (max_bytes_) in Linux EncodedRingBuffer, pruning packets when memory budget is exceeded. - Parse max_buffer_mb from argv[6] on Linux, reject configurations exceeding the measured memory budget, and log resource allocation. - Add native tests in linux_encoded_ring_duration_test for byte-measured memory guard pruning and dynamic limit clamping. - Update UI advisory popup to distinguish Windows disk streaming from Linux in-memory budget enforcement. - Clean up whitespace issues flagged by git diff --check across deduplicator files. - Require FFmpeg runtime in CI for deduplication integration tests rather than silently skipping. - Add retry logic with exponential backoff to tools/fetch_third_party.py to prevent CI download connection resets.
…add spool dir fallback
|
Im splitting this to multiple PRs rn. |
Victor7996
changed the title
kalopsia-space
left a comment
kalopsia-space
left a comment
There was a problem hiding this comment.
Review summary
Verdict: Changes requested. The current head is green, but this remains too broad and explicitly depends on other PRs (#42 and #43) while mixing a large cross-platform deduplication feature with Linux engine, Windows engine, and replay-buffer changes. Please split or rebase this into independently mergeable pieces before approval.
The deduplication safety work is substantially improved: deletion is opt-in, output collisions are protected, low-confidence timestamps require confirmation, and originals are quarantined. However, the UI still presents estimated savings during scan and the implementation only knows confirmed savings after a destructive/quarantine operation. Please make that distinction explicit in the preview/action wording, and ensure the merge path cannot report confirmed savings unless every intended original was actually quarantined.
The latest author comment says the work is being split; please push the resulting focused PR(s) and close or substantially narrow this one.
| item_word = "group(s)" if any(len(p.clips or []) > 2 for p in pairs) else "pair(s)" | ||
| self.info_label.setText( | ||
| f"Found {len(pairs)} overlapping clip {item_word}. " | ||
| f"Estimated potential disk savings: {total_saved_mb:.1f} MB.{note_str} " |
There was a problem hiding this comment.
Blocking clarity issue: this is an estimated value from metadata/overlap ratios, not confirmed disk savings. Label it explicitly as an estimate in the action/preview UI and reserve confirmed/recovered for the post-merge result after verifying every original was actually quarantined.
| quarantined = quarantine_original_clips(orig_paths) | ||
| merged_size = output_path.stat().st_size if output_path.exists() else 0 | ||
| freed = sum(orig_sizes.get(p, 0) for p in quarantined) | ||
| actual_saved = max(0, freed - merged_size) |
There was a problem hiding this comment.
Please verify the destructive result before reporting savings. If any original fails to quarantine, freed is reduced, but the merge still returns success; the UI can then report a successful merge/recovered space while some originals remain. Return an explicit partial-quarantine result or fail the destructive operation when the requested originals were not all moved.
…k settings matching
3 participants
What does this PR do?
Introduces the Clip Deduplicator & Overlap Manager tool for identifying, clustering, and losslessly merging overlapping video recordings across Windows and Linux.
Why?
When saving multiple instant replays during intense gameplay, overlapping recordings often create redundant content. This tool automatically identifies overlapping timeline segments and merges them using fast FFmpeg stream copy (
-c copy) without re-encoding or quality loss.Changes
FTHR_UI/core/clip_deduplicator.pyfor overlap detection, cluster chaining, and stream-copy merging while preserving multi-track audio.FTHR_UI/ui/deduplication_dialog.pyfeaturing cluster previews, 64-bit signal safety (qint64), and worker thread isolation.SHFileOperationWwithFOF_ALLOWUNDO) and Trash (gio trash/.fthr_quarantine) on Linux, with file-lock checks.tests/test_clip_deduplicator.py,tests/test_clip_deduplicator_integration.py,tests/test_deduplication_dialog.py).Testing
QT_QPA_PLATFORM=offscreen pytest tests/— all pass (1004 passed, 34 skipped)Notes
fix(linux): resolve VA-API vaMapBuffer2 crash on LTS distros and enforce ring buffer RAM guard) for Linux engine stability and PR feat: extend maximum replay buffer setting to 30 minutes #43 (feat: extend maximum replay buffer setting to 30 minutes) for extended buffer configurations.