Repository navigation
✨ feat(benchmark): compare models on one range - #171
Conversation
- ✨ Features:
- "Copy render settings" in the Render button's context menu
(Ctrl/⌘+Shift+C) copies the current request as versioned JSON. It is
registered on the window, so it also works while Render is disabled,
e.g. while a model is still downloading.
- `core/request_json.py` round-trips sampling (exact fractions), crop,
segmentation, framing, transform and `max_bytes`. Location and rebuild
flags stay with the caller.
- `scripts/benchmark_models.py` renders that request once per model
through `ProductionPreviewRuntime`, so weights still download through
the Qt transport. Defaults to `V1_MODEL_IDS`. `bria-rmbg` and
`u2net_cloth_seg` run only when named, and bria prints its licence note.
- Per model it records prepare and render time, the provider actually
used and any fallback notice. It flags cut-cache reuse and mismatched
frame counts or durations. Results are rewritten after every model, so
Ctrl+C keeps the finished ones.
- `scripts/benchmark_report.py` writes a self-contained `index.html` grid
with a background toggle and a synchronised restart.
- 🔄 Export and presenter share `_try_export_request`, which does not
validate the output location, so a bad output setting cannot disable the
export.
- 🌐 German and English catalogues carry the three new strings.
- 🧪 Tests: JSON round trip including transform, export reachability while
Render is disabled, requests identical across models except model and
provider, partial results on interruption, report escaping.
- 📝 README: "Comparing models" under "Run from source".
Refs #170
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| def _build_copy_render_settings_action(self) -> QAction: | ||
| """Build the export action, reachable while Render is disabled (issue #170). | ||
|
|
||
| Qt drops both a disabled widget's own shortcuts and the `ContextMenu` | ||
| events it would otherwise turn into `customContextMenuRequested` | ||
| (verified offscreen, with the Render button enabled and disabled) -- | ||
| so a `QAction` or a context-menu route that lives *on* the Render | ||
| button goes unreachable exactly when this command is most useful: | ||
| while a model download or a job is running and the button is | ||
| disabled. | ||
|
|
||
| The fix has two parts. The shortcut is a `WindowShortcut`-context | ||
| action that `MainWindow` registers on the window itself, not the | ||
| button, so its availability never depends on the button's enabled | ||
| state. The right-click route is delivered through an event filter |
There was a problem hiding this comment.
💡 Quality: New user-visible context menu lacks regenerated screenshot
This PR adds a user-visible UI feature — the Render button's "Copy render settings" context-menu entry, its Ctrl/⌘+Shift+C shortcut, a disabled-state tooltip, and a "Copied render settings to clipboard" confirmation toast. The custom review instructions require that user-visible features update the README and generate screenshots via scripts/screenshots.py (never hand-pasted). The README's "Comparing models" section was added, but no regenerated screenshot showing the new context-menu action is included in the change set. Consider running scripts/screenshots.py to capture the new menu/tooltip and reference it from the README, or confirm this developer-tooling entry is intentionally exempt.
Was this helpful? React with 👍 / 👎
- 🐛 `_ensure_qt_application` always built a `QGuiApplication`. That needs a display backend, so on a headless machine the documented `--request` path could abort before reading the file. It now builds a plain `QCoreApplication` for `--request`, which is enough for the download transport's event loop, and a `QGuiApplication` only when the settings come from the clipboard. An existing instance is reused. - 🧪 Two subprocess tests pin the application class per input source. Trigger: Codex review of #171 (P2). Both new tests failed before the fix. Refs #170
- ✨ Features:
- `format_summary_table` prints a table after every run, including an
interrupted one: model, prepare and render seconds, frames, size,
provider actually used, and status. Rows are sorted by render time
with failed models last. A provider fallback shows as
`CPU (fallback)`, and a cut-cache hit as `reused cuts`, because that
time is not a real measurement.
- `index.html` can sort tiles by render time, model name or file size.
The tiles are generated in render-time order, so the page is already
sorted without JavaScript.
- `sort_by_render_time` is shared by the table and the report, so both
use the same order.
- `run_benchmark` raises `BenchmarkInterrupted` carrying the models
finished before Ctrl+C.
- 📝 Code comments no longer refer to review item numbers.
- 🧪 Tests cover table order, the fallback and reused-cuts markers, the
sort control and data attributes, tile order, and the interrupted run.
Refs #170
|
Analysis results on Tests: all checks pass, including SonarCloud — quality gate fails on one finding, which we consider safe.
That path comes from whoever runs the script on their own machine, not from a request, a file or any other outside input. There is no privilege boundary to cross: the person invoking it can already write anywhere their account can. Constraining It is marked Safe in SonarCloud with that reasoning rather than worked around in code. No other new issues were reported. Gitar — the missing screenshot is intentional. Gitar asks whether the new "Copy render settings" entry is exempt from regenerating the README screenshots, and it is. The entry exists to feed Should the benchmark ever grow a surface in the product itself, the screenshots are generated by Qualification: a real run on a Windows machine with DirectML rendered all 13 default models without an error or a fallback — 86 frames and 5733 ms each, so the outputs are directly comparable, and no run reused an existing cut set. Render time ranged from 109 s (u2net) to 177 s (birefnet-general). About 100 s of that is decoding and lossless WebP encoding, which every model pays alike. |
- 🐛 The Render button's context menu drew "Ctrl+Shift+C" on top of the "Copy render settings" label. Qt's windows11 style, the default on Windows 11 since Qt 6.7, under-sizes `CT_MenuItem` for this label, shortcut and font unless a stylesheet rule owns the item's box model. `_control_stylesheet` now styles `QMenu` and `QMenu::item`, which moves the sizing onto Qt's stylesheet path. The palette follows the existing combo-box popup. - 🧪 The test renders the menu offscreen and measures the gap between the label's ink and the shortcut's ink, because `sizeHint()` does not expose the defect: the buggy hint (220px) is already wider than the text it holds (193px). Measured gap 4px before the fix, 12px after, and the test skips where the windows11 style is absent. Trigger: repro — right-click the Render button on Windows 11; the shortcut sits on the label. The test fails without the stylesheet rule. Refs #170
- ✨ Features:
- Thirteen lossless animated WebPs playing at once made the report lag.
An `IntersectionObserver` now keeps the animation on visible tiles and
swaps the rest to a still frame, a click toggles one tile, and the
column count is selectable and remembered.
- `benchmark_stills.py` extracts frame 0 of each WebP as a PNG for that
fallback. A tile whose still cannot be extracted keeps its animation.
- `--report-only DIR` rebuilds the report and the stills from an
existing `results.json` and the WebPs beside it, without a render, a
model or a Qt GUI application.
- Without JavaScript the page keeps the animated sources, as before.
- ⚙️ `/benchmark-*/` is ignored, so a run's WebPs cannot reach a commit.
- 🧪 Tests cover the still filenames, a tile without a still, the new
controls, the rebuild from `results.json` and its error on a missing
file.
- 📝 README names `--report-only`.
Refs #170
|
Code Review 👍 Approved with suggestions 0 closed / 1 findingsBenchmark feature that renders one range across all segmentation models with results aggregated in an HTML grid. The new "Copy render settings" context menu entry (Ctrl/⌘+Shift+C) lacks a regenerated screenshot in the README — consider running 💡 Quality: New user-visible context menu lacks regenerated screenshot📄 README.md 📄 src/matteloop/ui/action_shelf.py:168-182 This PR adds a user-visible UI feature — the Render button's "Copy render settings" context-menu entry, its Ctrl/⌘+Shift+C shortcut, a disabled-state tooltip, and a "Copied render settings to clipboard" confirmation toast. The custom review instructions require that user-visible features update the README and generate screenshots via scripts/screenshots.py (never hand-pasted). The README's "Comparing models" section was added, but no regenerated screenshot showing the new context-menu action is included in the change set. Consider running scripts/screenshots.py to capture the new menu/tooltip and reference it from the README, or confirm this developer-tooling entry is intentionally exempt. 🤖 Prompt for agentsReview coverageRules No rules evaluated OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Important Your trial ends in 2 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
2 participants
What
A developer benchmark that renders one range with every segmentation model, with every other setting held constant, and puts the results side by side.
scripts/benchmark_models.pyrenders that request once per model through the app's own runtime and writes:results.jsonwith preparation and render times, the provider actually used, and warnings,index.htmlwith every WebP in a grid.Decisions worth reviewing
V1_MODEL_IDS(13 models).bria-rmbghas its own licence andu2net_cloth_segneeds a clothing-category input, so both run only when named explicitly with--models. Selecting bria prints its licence note first.CutWorkspacerequires the canonical workspace root, and that root cannot be injected without changing the frozenjobs/render.py.--regeneratewas removed: it would re-promote a user's hand-edited cut set.max_bytes, which decides between lossless and lossy encoding. Only the output location and the rebuild flags are left out.QtNetworkDownloadTransportnever dispatches, and the first uncached model hangs.Verification
ruff,mypy(host, linux, win32) andcheck_guardrails.pypass.mainshows on the same machine. The branch adds none and removes none. They come from Windows path separators, CRLF line endings, and a missing PySide font directory.test_render_command_publishes_a_lossless_animated_webpwas deselected, because it segfaults the full run on this machine. It does the same onmain, and it passes when run on its own.test_fixed_shelf_only_contains_reachable_primary_actions_at_minimumalready fails locally onmain, so only CI shows whether the new context menu affects it.Closes #170