Audit fixes + features: release repair, data-integrity, Windows, save/load, live-preview, engine#10
Conversation
…landmine The published download portal (docs/index.html) linked to artifact names the release workflow never produces, so all 7 download buttons 404'd. The actual v1.1.5 release assets are named `Datamosh-<version>-<os>-<installer|portable>.*` (plus `-linux-x86_64.AppImage`). - docs/index.html: rewrite all 7 download links to the real asset names. Verified: each now returns HTTP 200 against releases/latest. - pages.yml: add a deploy-time step that rewrites the versioned asset names and the "download vX" tag from VERSION, so the links can never drift again. - Delete update_beta5_release.yml: on any RELEASE_BODY.md edit it force-set v1.1.0-beta.5 as GitHub's "latest" release (make_latest:true), which would override v1.1.5 for every releases/latest/download link. - Add ci.yml: run the pytest suite (offscreen) on push/PR across ubuntu+windows; previously nothing gated commits before tagging/shipping. - CHANGELOG.md: add the missing [1.1.5] section (the 12 fixes shipped in 1.1.5). - ROADMAP.md: mark the import dialog (v1.1.1) and icon toolbar as shipped; list project save/load as the top near-term priority. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mp cleanup undo-safe Three silent data-integrity bugs in the clip/selection layer: 1. Removing the selected non-last clip left the settings panel bound to the deleted row. remove_clip computed new == old _selected_row, and select_clip early-returned on an unchanged index, so clip_selected never re-fired and the panel kept writing edits to whatever clip now occupied the slot. Fix: rebuild the selection for any removal at/before the selection and force a clip_selected re-emit (new `force` arg on select_clip). 2. Clip-bin drag-reorder bypassed Project: ClipListModel.dropMimeData mutated the list directly, so the reorder was not undoable, never emitted clips_changed (preview/timeline didn't refresh), and left _selected_row pointing at the wrong clip. Fix: dropMimeData now emits reorder_requested; Project._on_clips_reordered records undo, performs the move, rebinds selection to the moved clip by identity, and emits clips_changed. 3. remove_clip eagerly rmtree'd the clip's normalized temp dir, but the undo snapshot still held that ClipProfile (is_ready()==True), so undo restored a clip whose media file was gone -> render/preview failed on a missing file. Fix: defer temp-dir cleanup. Removed clips go to a pending list and are reaped only once unreachable from the live list and both history stacks; cleanup_all() (wired into MainWindow.closeEvent) reclaims everything on exit. Adds tests/test_clip_selection_regressions.py (8 tests). Full suite: 66 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…be, launcher
Make the app behave natively on Windows without touching mosh.py:
- gui/ffmpeg_env.py: central helper module.
* suppress_subprocess_console() installs a subprocess.Popen subclass that adds
CREATE_NO_WINDOW on Windows (run/call/check_* all funnel through Popen), so
every ffmpeg/ffprobe call — including the ones inside mosh.py — stops flashing
a console window in the --windowed build. Idempotent, no-op off Windows.
* ensure_ffmpeg_on_path() prepends a bundled "ffmpeg" dir (frozen builds) to PATH
so bare ffmpeg/ffprobe resolve to shipped binaries.
* missing_ffmpeg_tools() drives a clear startup QMessageBox (main.py) with install
instructions when ffmpeg/ffprobe are absent — previously a fresh install just
appeared broken with the failures swallowed.
- release.yml (Windows): download + bundle ffmpeg.exe/ffprobe.exe via --add-binary
into the bundle's ffmpeg/ dir (pinned GyanD build; GPL is fine for GPL-3.0).
- clip_panel.py: move the direct-import codec probe to a CodecProbeWorker thread;
the old synchronous ffprobe blocked the UI thread up to 5s on AVI import.
- timeline_widget.py: build fonts from the platform default + SansSerif style hint
instead of QFont("Sans", ...), which isn't a real family on Windows/macOS.
- normalize_worker.py / iframe_inject_worker.py: rmtree their mkdtemp dir on failure
so a failed ffmpeg run doesn't leak a temp dir in %TEMP%.
- launch.bat: Windows launcher mirroring launch.sh.
Adds tests/test_ffmpeg_env.py. Full suite: 71 passed. Offscreen MainWindow
construct/show/close smoke-tested.
Note: the release.yml ffmpeg bundling step needs a real tagged CI build to validate
end-to-end (can't run Windows PyInstaller + the download locally).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…r on >4GB output Two core-correctness bugs in the moshing path: 1. Cut/inject at playhead cut at the wrong place when duplication/drops were active. ClipRegion.frame_count is the predicted OUTPUT count and duration_sec is derived from it, but current_playhead_location() computed the cut frame as int(local_sec * fps) — an OUTPUT index — which project.split_timeline_item then interpreted against the SOURCE frame count. With e.g. 100 source frames doubled to 200, dropping the playhead at the visual midpoint produced frame ~100, which split clamped to the source end (99) — cutting at the very end instead of the middle. Fix: track source_frame_count on ClipRegion and map the playhead's time fraction within the segment to a SOURCE frame. (The preview-frame mapping in _emit_frame_for_sec stays output-based — correct, since preview shows output.) 2. Large/high-duplicate moshes overflowed the 32-bit AVI size fields in mosh.py and surfaced the raw "'I' format requires 0 <= number <= 4294967295" struct.error in the render dialog. mosh.py is treated as untouched, so MoshWorker now catches struct.error and emits an actionable "~4 GB limit — reduce duplicate count / clip length" message instead. Adds tests/test_timeline_cut_mapping.py (3) and a worker overflow-message test. Full suite: 75 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adversarial review of the four fix commits surfaced small correctness gaps: - pages.yml: the version auto-sync only ran on docs/** pushes, so a future release would leave the portal showing stale (404-ing) asset names. Trigger it on VERSION changes, on release: published, and via workflow_dispatch; also sync the hero version label (previously only the links + download tag were rewritten). - project.py: dedup _pending_temp_cleanup by object identity (was value-equality on the ClipProfile dataclass — inconsistent with the rest of the identity-based reaping). - project.py: after removing a clip that backs timeline segments, follow the still- present selected segment to its new index instead of only clamping when out of range — fixes selection pointing at the wrong segment when an earlier one is removed. - mosh_worker.py: only map struct.error to the "~4 GB limit" message when its text is the integer-overflow signature; a struct.error from parsing a truncated/corrupt AVI now reports a parse error instead of a misleading size message. Adds 2 regression tests (mid-timeline segment reselection; non-overflow struct.error not mislabeled). Full suite: 77 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add full session persistence — the audit's top missing feature. Previously all
timeline/clip work was lost on close.
- gui/models/project_io.py: serialize/read/write a .dmosh JSON file (format +
version guarded). Stores each clip's source path + per-clip mosh settings, the
timeline arrangement (in/out/drop-override), selection, and the import settings.
Normalized/temp media is NOT stored — it's regenerated on load, keeping files small.
- ClipProfile.source_kind ("clip"/"iframe") so injected single-frame clips round-trip:
on load they are rebuilt from their source media via the inject worker; normal clips
are re-normalized. clip_panel.reingest_loaded_clip() branches on kind.
- Project.clear() and install_loaded_state(): reset/rebuild clips+timeline+selection
with history cleared (a load is a clean slate) and temp dirs reclaimed.
- preview_widget.reset(): cancel in-flight work and clear the display on new/load.
- MainWindow: File > New / Open Project / Save / Save As (Ctrl+N, Ctrl+Shift+P,
Ctrl+S, Ctrl+Shift+S); unsaved-changes tracking (title shows name + '*'); discard
prompt on new/open/quit; missing-source reporting on load.
- README: document projects + new shortcuts.
Dirty tracking keys off rowsInserted/rowsRemoved + timeline/settings edits, so async
re-ingest after a load does not falsely mark the project dirty.
Adds tests/test_project_io.py (9 tests). Full suite: 86 passed; end-to-end
add->save->new->load orchestration smoke-tested offscreen.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…preview The live re-mosh pipeline reused one preview.avi and never disconnected superseded workers, so a rapid setting/selection change could let an old MoshWorker's finished_ok or an old FrameExtractor's frame_ready fire after the next cycle started — mixing frames from two outputs — and cancellation used QThread.terminate() (unsafe on a CPython thread, and could truncate the shared preview.avi mid-write). - Generation token: _cancel_workers() bumps a counter; every worker callback (_on_mosh_done / _on_frame_decoded / _on_extraction_done / _on_mosh_error) is tagged with the generation it was started in and returns early if superseded. Stale callbacks are now no-ops, so frames can never mix and reset()/load can't be resurrected. - Unique temp file per generation (preview_<gen>.avi) + best-effort purge: a detached worker writes its own file and never clobbers the current preview; no shared-file race. - No more terminate(): the extractor is stopped cooperatively (it polls _abort) and the mosh worker is detached to finish into its own file and self-retire via `finished`. shutdown() waits for any in-flight worker so no QThread is destroyed while running. - Scrubber: _update_scrub_range() blocks signals and skips updates while the slider is being dragged, so decoding frames no longer fights an active scrub. Adds tests/test_preview_widget.py (5 tests). Full suite: 91 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…l ffmpeg errors mosh.py is no longer treated as untouchable (constraint lifted). Robustness fixes, all with round-trip behavior preserved (tests/test_mosh_engine.py): - idx1 is now advisory: parse the movi structurally and use idx1 ONLY for the per-chunk keyframe flag. Offsets and sizes are ignored (both rebuilt on write), and missing/extra/reordered/absent idx1 is tolerated instead of fatal. This accepts spec-valid AVIs with file-relative idx1 offsets and OpenDML files with no legacy idx1 — both of which the old strict 1:1 lockstep walk rejected outright. - Keyframe keep/drop precedence is now explicit and documented: explicit drop > explicit keep > drop_first > keep-limit. An explicitly kept ordinal 0 now survives drop_first; keep∩drop resolves to drop. Default behavior (no explicit sets) is unchanged. - normalize_to_xvid captures ffmpeg stderr and surfaces the tail in the error (was just "exit code N"), with a targeted hint when the build lacks the libxvid encoder — the common Windows failure. - Output is capped at the 4 GB AVI limit at the source with a clear AviParseError, instead of letting a raw struct.error escape. - CLAUDE.md updated to match (mosh.py may be modified carefully; keep tests green). Adds tests/test_mosh_engine.py (13 tests, incl. a minimal in-memory AVI builder). Full suite: 104 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adversarial review of the save/load, live-preview, and core-engine commits found real defects on the project-change and shutdown paths: - Cooperative MoshWorker abort (closes the terminate()-removal crash window). mosh.py process_chunks/rewrite_avi gained an optional should_abort callback that raises MoshAborted at the next chunk; MoshWorker.abort() sets the flag and run() swallows MoshAborted (emits nothing). preview _cancel_workers now aborts the mosh worker too; reset() waits for aborted workers before returning so clear()/load can't rmtree a temp dir a worker is still reading; shutdown() keeps a terminate() last resort at process exit only (a worker that won't stop no longer gets destroyed-while-running). - Open-over-session leak: install_loaded_state() now calls cleanup_all() up front, so File>Open reclaims the previous session's temp dirs (was only done by File>New). - Stale ingest callbacks: ClipPanel gained an ingest epoch; New/Open calls cancel_ingest() to invalidate in-flight normalize/probe/inject callbacks so a late completion can't cross-wire media onto a freshly-loaded clip, and orphaned temp output is reaped. - Scrubber: re-applies range/label on sliderReleased (updates are skipped mid-drag). - 4 GB guard now also covers the idx1 size field (AviParseError, not raw struct.error). Adds 6 tests (process_chunks abort, open-over-session cleanup, cancel_ingest/orphan). Full suite: 110 passed; New/Open orchestration re-smoke-tested. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces project persistence to save and load .dmosh project files, moves video and codec probing off the main thread to prevent UI freezes, and implements robust temp-directory cleanup and drag-reorder synchronization. It also adds Windows-specific console suppression for subprocesses and enhances the AVI parser to tolerate missing or absolute-offset indexes. The review feedback highlights three key improvement opportunities: robustly scanning forward in idx1 to prevent parser desynchronization on extra entries, checking for the presence of 'creationflags' in kwargs to avoid overriding explicitly passed falsy flags, and wrapping the project loading sequence in a broader exception handler to gracefully handle malformed data.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| flags = 0 | ||
| if entry_idx < len(idx_entries): | ||
| idx_chunk_id, idx_flags, _idx_offset, _idx_size = idx_entries[entry_idx] | ||
| if idx_chunk_id == chunk_id: | ||
| flags = idx_flags | ||
| entry_idx += 1 |
There was a problem hiding this comment.
The advisory idx1 matching logic uses a strict lockstep walk (entry_idx += 1 only on match). If idx1 contains an extra entry (e.g., a chunk that was dropped from movi but left in the index), idx_chunk_id == chunk_id will evaluate to False, and entry_idx will never advance. This permanently desynchronizes the parser, causing all subsequent keyframes in the file to be missed. Scanning forward from entry_idx to find the next matching entry resolves this robustly without getting stuck.
| flags = 0 | |
| if entry_idx < len(idx_entries): | |
| idx_chunk_id, idx_flags, _idx_offset, _idx_size = idx_entries[entry_idx] | |
| if idx_chunk_id == chunk_id: | |
| flags = idx_flags | |
| entry_idx += 1 | |
| flags = 0 | |
| for scan_idx in range(entry_idx, len(idx_entries)): | |
| idx_chunk_id, idx_flags, _, _ = idx_entries[scan_idx] | |
| if idx_chunk_id == chunk_id: | |
| flags = idx_flags | |
| entry_idx = scan_idx + 1 | |
| break |
| def __init__(self, *args, **kwargs): | ||
| if not kwargs.get("creationflags"): | ||
| kwargs["creationflags"] = _CREATE_NO_WINDOW | ||
| super().__init__(*args, **kwargs) |
There was a problem hiding this comment.
In _NoWindowPopen.__init__, checking not kwargs.get('creationflags') will override creationflags if it is explicitly passed as 0 (which is falsy). This prevents callers from explicitly requesting a console window (or passing other falsy flags if any exist). It is safer to check if 'creationflags' is present in kwargs using the in operator.
| def __init__(self, *args, **kwargs): | |
| if not kwargs.get("creationflags"): | |
| kwargs["creationflags"] = _CREATE_NO_WINDOW | |
| super().__init__(*args, **kwargs) | |
| def __init__(self, *args, **kwargs): | |
| if "creationflags" not in kwargs: | |
| kwargs["creationflags"] = _CREATE_NO_WINDOW | |
| super().__init__(*args, **kwargs) |
| def _load_project_file(self, path: Path) -> None: | ||
| try: | ||
| data = project_io.read_project(path) | ||
| except project_io.ProjectLoadError as exc: | ||
| QMessageBox.critical(self, "Open Project", str(exc)) | ||
| return | ||
|
|
||
| self._loading = True | ||
| try: | ||
| self.clip_panel.cancel_ingest() | ||
| self.preview_widget.reset() | ||
| settings = data.get("import_settings") or self.clip_panel.current_import_settings() | ||
| clips = self.project.install_loaded_state(data) | ||
| for row in range(len(clips)): | ||
| self.clip_panel.reingest_loaded_clip(row, settings) | ||
| finally: | ||
| self._loading = False |
There was a problem hiding this comment.
In _load_project_file, only project_io.ProjectLoadError is caught. If the project file contains malformed data or unexpected types (e.g., a string instead of an integer for keep_first), self.project.install_loaded_state(data) or self.clip_panel.reingest_loaded_clip can raise a ValueError, TypeError, or KeyError. These unhandled exceptions will propagate and could leave the application in an inconsistent or partially broken state. Wrapping the entire loading sequence in a try...except Exception block ensures robust error handling for any corrupted project files.
| def _load_project_file(self, path: Path) -> None: | |
| try: | |
| data = project_io.read_project(path) | |
| except project_io.ProjectLoadError as exc: | |
| QMessageBox.critical(self, "Open Project", str(exc)) | |
| return | |
| self._loading = True | |
| try: | |
| self.clip_panel.cancel_ingest() | |
| self.preview_widget.reset() | |
| settings = data.get("import_settings") or self.clip_panel.current_import_settings() | |
| clips = self.project.install_loaded_state(data) | |
| for row in range(len(clips)): | |
| self.clip_panel.reingest_loaded_clip(row, settings) | |
| finally: | |
| self._loading = False | |
| def _load_project_file(self, path: Path) -> None: | |
| try: | |
| data = project_io.read_project(path) | |
| self._loading = True | |
| try: | |
| self.clip_panel.cancel_ingest() | |
| self.preview_widget.reset() | |
| settings = data.get("import_settings") or self.clip_panel.current_import_settings() | |
| clips = self.project.install_loaded_state(data) | |
| for row in range(len(clips)): | |
| self.clip_panel.reingest_loaded_clip(row, settings) | |
| finally: | |
| self._loading = False | |
| except Exception as exc: | |
| QMessageBox.critical(self, "Open Project", f"Failed to load project:\n{exc}") | |
| return |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 314a81d41d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| elif clip_drop_first and clip_key_index == 0: | ||
| keep = False |
There was a problem hiding this comment.
Mirror keep/drop precedence in timeline predictions
With this new precedence, explicitly keeping keyframe 0 now wins over drop_first_keyframe, but TimelineWidget._predict_output_frame_count and the frame visualization still test drop_first before keep_set. For a clip or segment with “Drop first I-frame” enabled and keep_keys_spec='0', the engine keeps that frame while the timeline duration/playhead mapping predicts it was removed, so scrubbing and cut positions are off by at least one frame. Please update the timeline predictor/viz to use the same precedence.
Useful? React with 👍 / 👎.
| self.project.clip_model.rowsInserted.connect(self._mark_dirty) | ||
| self.project.clip_model.rowsRemoved.connect(self._mark_dirty) |
There was a problem hiding this comment.
The new dirty tracking only listens to clip updates, timeline changes, and insert/remove events. Drag-reordering the clip bin goes through Project._on_clips_reordered, which performs beginMoveRows/rowsMoved and emits only clips_changed, so none of these connections mark the project dirty. Since .dmosh serialization writes project.clips in order, a user can reorder clips and close/open without a save prompt, losing the saved order unless they make another edit.
Useful? React with 👍 / 👎.
Addresses several open issues: - Fixes #7 (no sound, even in exported video): normalize_to_xvid re-encodes audio to MP3 (libmp3lame) instead of `-c:a copy`. AVI can't reliably carry the AAC/Opus audio in MP4/WebM/MOV sources, so copying produced silent/unplayable output. MP3 is the AVI-compatible codec. - Fixes #3 (playhead doesn't snap to frames): dragging the playhead now snaps to ~1/3 into the frame under the cursor (TimelineCanvas._snap_sec_to_frame) — clearly inside one frame and left-of-centre so the cut side is predictable, exactly as requested. - Addresses #5 (macOS hotkeys): toolbar tooltips render the shortcut in platform-native form (⌘ on macOS) instead of hardcoded "Ctrl", and the timeline delete now also binds Backspace (macOS's primary delete key). Functional Ctrl→⌘ mapping was already handled by Qt. (Can't verify on macOS from here — needs a Mac to confirm.) - Fixes #4 (cut misplaced when duplicates+gap set) was already resolved earlier in this branch via the output→source frame-index mapping; covered by test_timeline_cut_mapping. - Addresses #8 (some videos import pre-glitched/errored): the audio re-encode + idx1 tolerance + real ffmpeg-stderr reporting (earlier in this branch) help; a full fix needs the specific source file. Adds tests (audio cmd, frame-snap, native tooltip). Full suite: 115 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The engine only writes AVI. The render dialog now offers AVI (native) / MP4 / MOV; for MP4/MOV the worker moshes to a temp AVI and transcodes it with ffmpeg (libx264 + aac), re-encoding the glitched frames into a clean, shareable file. Native AVI export is unchanged (no transcode). Adds worker tests for both paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Glitch settings are now per-timeline-segment, not just per-clip — so the same clip can appear twice with different mosh settings (no more importing a clip twice to duplicate a section). - TimelineItem gains keep_first/duplicate_count/duplicate_gap/keep_keys/drop_keys overrides (None = inherit the clip; drop-first was already per-segment). - timeline_render_clips() applies the overrides onto each segment's render copy, so both preview and render honour them. - The settings panel edits the selected *segment* (storing overrides) and displays its effective values; with no segment selected it still edits the clip. - update_timeline_item_settings() is undoable; snapshots now carry the overrides (timeline snapshot entries switched from positional tuples to dicts). - Overrides persist in .dmosh (serialize + load). Adds tests (render applies override, undoable, .dmosh round-trip). Full suite: 120 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Frames were drawn at a uniform width (clip_w / source_frames), so with a duplicate gap every frame looked equally long and kept I-frames got stretched. Now each source frame is laid out proportional to its OUTPUT contribution: a duplicated P-frame spans (1 + dup_count) units and draws visibly longer, a normal frame spans 1, a kept I-frame spans 1 (no stretch), and a dropped keyframe spans 0 (thin marker). The orange marker spans the whole widened duplicated frame. Also wires per-segment overrides (#2) into the timeline: refresh() now feeds each region its *effective* keep/dup/gap/keys settings, so the viz and duration reflect the segment. Adds a region-reflects-segment-override test. Full suite: 121 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comprehensive pass from a multi-dimension audit of v1.1.5. 9 commits, 110 tests (was 58),
mosh.pyround-trip preserved.Fixes & features
v1.1.0-beta.5as "latest", addedci.yml(pytest on push/PR, ubuntu+windows), version-drift guard inpages.yml, CHANGELOG 1.1.5 + ROADMAP accuracy.CREATE_NO_WINDOWfor all ffmpeg/ffprobe calls (no console flashes), bundled+detected ffmpeg with a clear startup dialog, async codec probe (no UI-thread freeze), portable fonts, temp-leak-on-failure fixes,launch.bat.struct.error..dmoshprojects — full session persistence (clips + per-clip settings + timeline + selection); media regenerated on load; File ▸ New/Open/Save/Save As, dirty-tracking title.terminate()); scrubber no longer fights active drags.mosh.py) — idx1 now advisory (tolerates absolute offsets / missing / extra / no-idx1); explicit keep/drop precedence; real ffmpeg stderr surfaced (+libxvid hint); source-level 4 GB guard.Two adversarial review passes; all findings addressed. Verified by running the app (clips import/normalize, live preview, timeline analysis all working).
🤖 Generated with Claude Code