Add TheNoise-based upscalers - #3327
Conversation
ramkrishna2910
left a comment
There was a problem hiding this comment.
The refactor here is the right call and I'd like to land it — moving the binary-path resolution, backend selection, and CLI shelling out of server.cpp and into each backend is exactly where that code belongs, and the net −112 lines in server.cpp is a real improvement. I checked that the sd-cpp path survives the move intact: SDServer::upscale_via_cli preserves the full selection chain (explicit sd-cpp_backend config → SystemInfo::get_supported_backends auto-detect → cpu fallback), so the "silently falls back to CPU" hazard the old inline comment warned about is still guarded. It also now resolves the ROCm channel, which the inline version in server.cpp did not — a latent fix that came along for free.
The five new models are well-formed: recipe: "thenoise" resolves against backend_versions.json, and the upscaling + image labels are right.
Two things I'd like addressed before this merges, both small.
1. upscale_factor is a new request field on a documented endpoint, and it isn't documented.
POST /v1/images/upscale has a full entry in docs/api/openai.md (request table, examples in three languages). This PR adds upscale_factor to that request body with no corresponding documentation — I grepped, there are zero references to it anywhere under docs/. We're trying to hold the line that every endpoint and its parameters are documented, so this needs a row in that table.
2. upscale_factor is silently ignored by sd-cpp, which makes the docs harder to write honestly.
SDServer::upscale_via_cli takes the parameter as double /* upscale_factor */ and never uses it — only TheNoiseServer honors it. So the same field on the same endpoint either works or is silently discarded depending on which recipe the selected model happens to use, with nothing in the response indicating which happened.
Please either wire it through to sd-cli, or reject it with a clear 400 when the target recipe can't honor it. Silently dropping a caller-supplied parameter is the one option I'd rather avoid. Whichever you pick, the doc row from point 1 should say so explicitly.
Smaller notes, none blocking:
std::to_string(double)is locale-sensitive. InTheNoiseServer::upscale_via_clithe factor is formatted withstd::to_string, which followsLC_NUMERIC— under a comma-decimal locale that emits2,5and hands--upscale-factor 2,5to the CLI. It also always emits six decimals. Nothing insrc/cpp/callssetlocaletoday so this is latent rather than live, but it's the same trap #3169 hit withstd::stod, and #3100 already reaches forstd::locale::classic()for exactly this reason. Anostringstreamwithstd::locale::classic()would make it robust.- No upper bound on the factor.
infpasses the> 0.0guard and becomes--upscale-factor inf; a large finite value gets passed straight through to a subprocess that will try to allocate the result. A sanity ceiling on a public endpoint seems worth it. (NaNis already excluded, sinceNaN > 0.0is false.) - The dispatch stayed behind.
handle_image_upscalenow doesif (recipe == "thenoise") ... else if (recipe == "sd-cpp"). That's a hardcoded recipe switch inserver.cpp— the same shape of backend-specific knowledge the rest of this PR is removing. A capability interface alongside the others inserver_capabilities.hwould finish the job. Not for this PR, but worth an issue. - The
"rocm"variant is hardcoded inTheNoiseServer::upscale_via_cli. That matches reality today (it's the only variant inbackend_versions.json), but it will silently keep selectingrocmif a vulkan or cuda variant is ever added. A comment noting the ROCm-only assumption would save the next reader the lookup.
Coordination note: this overlaps with #2842, which adds model-level auto-upscale on the generation endpoints. The two conflict in server.cpp, and I think this one should land first — #2842's do_upscale currently dispatches on the upscaling label alone and then hardcodes sd-cli, so the five recipe: "thenoise" upscalers this PR adds (all suggested: true) would pass its check and then be handed to the wrong binary. The recipe dispatch you added here is the fix; once this is in, #2842 can route through it. I've flagged that on the other PR.
|
@ramkrishna2910 thanks for your review! I agree on all points. I decided to go ahead and remove the newly introduced Regarding the dispatch, I will do a followup PR. Regarding the hardcoded rocm variant, I added a comment so that it doesn't go unnoticed if new variants are added. |
ramkrishna2910
left a comment
There was a problem hiding this comment.
Approving. Both blocking items are resolved, and I built and ran this locally before signing off.
Removing upscale_factor outright was the better call than either option I offered — a parameter that one backend honors and the other silently discards is worse than no parameter, and dropping it means there's nothing to document inconsistently. The ROCm-only comment on the hardcoded variant is exactly what I was after.
Local verification at d4eb776f2, Windows / Visual Studio 2026 (vs18 preset, Release):
| Step | Result |
|---|---|
lemond build |
clean |
cpp-ci-tests build (~55 binaries) |
clean |
ctest -L cpp-ci |
55/55 passed, 0 failed (58.8s) |
Server start + /api/v1/health |
OK |
The only compiler warnings were two pre-existing NOMINMAX redefinitions in files this PR doesn't touch.
Two runtime checks worth recording, since they exercise the refactor rather than just compiling it:
POST /v1/images/upscalewith allamacppmodel returns{"message":"Upscale is not supported by recipe: llamacpp","type":"invalid_request_error"}— that's the newelsebranch, confirmed live.- An sd-cpp model reaches binary resolution under
bin/sd-cpp/vulkan, which means the selection chain you moved out ofserver.cpp(explicitsd-cpp_backend→SystemInfo::get_supported_backends→cpu) still resolves correctly at runtime. That was my main worry about the move and it holds.
What I could not verify: the TheNoise path itself never executes on my machine. None of the 12 thenoise models appear in /v1/models?show_all=true — including the seven that already exist on main — because the backend is ROCm-only and backend-availability filtering drops them. So TheNoiseServer::upscale_via_cli is covered by your testing and by review, not by mine. Flagging it so the gap is on the record rather than implied away.
One observation, not a blocker and not caused by this PR: handle_image_upscale doesn't enforce the upscaling label. I passed SD-Turbo — a generation model labelled image — and it was accepted as an upscaler and handed to sd-cli. I checked main and the guard isn't there either, so this is pre-existing. Worth knowing because #2842's do_upscale does check the label, so once both land the same bad input is rejected on the auto path and accepted on the direct endpoint. That belongs with the capability-interface cleanup rather than here — I'll open an issue.
Merge order: please land this before #2842. Its do_upscale dispatches on the upscaling label alone and then hardcodes sd-cli, so the five recipe: "thenoise" upscalers you're adding (all suggested: true) would pass its check and be handed to the wrong binary. The recipe dispatch you built here is the fix, and I've asked that PR to route through it.
Thanks for turning this around quickly.
|
Not ready for review yet To-dos
ExplanationDescription vs. the diff: accurate The PR body accurately describes adding TheNoise upscaling, the sdcpp refactor away from server.cpp, the removal of 'experimental' from the backend name, and the addition of the rocm bin variant; the diff matches all claims. Checked: The PR body claims upscaling for TheNoise, the sdcpp refactor, removal of 'experimental', and the rocm bin variant; all are confirmed in the diff (thenoise_server.cpp adds upscale_via_cli, server.cpp dispatches by recipe, thenoise.h changes the display name and bin_variants, server_models.json adds five model entries). Focus: focused All changes serve a single goal — enabling TheNoise-based upscaling. The server.cpp refactor, the sdcpp backend restructuring, the thenoise backend extension, the version bump, and the new model entries are all necessary for this one feature. Checked: The diff touches server.cpp (refactoring the upscale endpoint), thenoise_server.cpp (new upscale_via_cli), sdcpp_server.cpp (receiving the refactored upscale logic), thenoise.h (display name and bin variant), backend_versions.json (version bump), server_models.json (new models), and related docs — all serving the single goal of TheNoise upscaling. Alignment: none found Checked: reviewed contribute.md and philosophy.md; the change is a contained extension of the existing /upscale surface with no new endpoints or commands. Documentation: gaps
Testing: gaps
Breaking changes: none found Checked: no user-facing surfaces are removed or renamed; the SDServer::upscale_via_cli signature change is internal code structure. Review suggestion Attention level: Routine — needs any 1 reviewer. Already reviewed by: ramkrishna2910. ExplanationWhy this rung A contained fix or extension. The diff is a contained fix or extension under code structure — it adds upscaling support to the existing thenoise backend and refactors the upscaling endpoint in server.cpp to dispatch by recipe. This is one-reviewer per the table. Reviewer search Checked: ramkrishna2910 (code-author on server_models.json/sdcpp_server backend registration), superm1 (ROCm maintainer), bconsolvo (original sd-cpp upscaling author); thenoise is bitgamma's area but the author is excluded. [AI-assisted review] Automated pre-review from repo-manager — flags for the human reviewer, not a replacement for one. Reviewed at head |
|
will make an issue for the above since its coming in late |
* update thenoise version * add support for thenoise in the upscaler endpoint * bump version and better model names * update documentation * remove upscale_factor * notice about backend selection
Summary
Adds the ability to upscale images using TheNoise + adds additional upscaling models. Since the upscaling endpoint was hardcoded for sdcpp, a few changes were needed to sdcpp backend to move backend-specific code away from
server.cpp.Additionally I removed "experimental" from the backend's name and added the "rocm" bin variant to users can keep track of specific forks or "latest" like the other backends.
Scope
Testing
In image generation, select one of the upscaler models from this PR (-TheNoise suffix). Also tried the pre-existing models and both paths still work.
Documentation
Breaking Changes
AI-assisted contribution
Please select one:
If AI tools were used: