[Stable Diffusion] Enable image upscaling for external calls - #2842
[Stable Diffusion] Enable image upscaling for external calls#2842NineBallo wants to merge 11 commits into
Conversation
|
This one has a few possibly polarizing decisions, opening for review but just lmk where you want to go with it. I think it makes a lot of sense but idk. |
|
[AI-assisted review] Automated pre-review from repo-manager — flags for the human reviewer, not a replacement for one. Attention level: Elevated — any reviewer can take it, but the to-dos below need resolving before approval (2 documentation gap(s)) Documentation gaps
Scopeminor — The PR extends the existing sd-cpp recipe with a new optional Suggested reviewers
Reviewed at head |
|
@NineBallo please see TODOs in the comment above @ramkrishna2910 I think you should review |
|
Also @NineBallo please be aware that we have a feature freeze on gui2, and the gui changes will need to go into GUI3. If you can remove the gui features from this PR and rebase them into gui3 in a future PR that would be great. |
|
@jeremyfowers Oops totally forgot about the docs! Some of these pr's won't be directly usable in that case but I'll strip ui changes out and open up seperate prs. GUI PR questions:
|
Wait for gui3 to merge to main first
@kpoineal - heads up there's 3 PRs adding some minor UI elements to the image gen UI. |
|
Thanks for picking this up @NineBallo — the gap in #2836 is real, and I agree the Some context that's relevant here: an earlier iteration of the upscaling work did have exactly this design — an Concerns I'd like to talk through before this lands:
I want us to make that tradeoff on purpose, and to not lose the progress reporting and side-by-side comparison in the process. |
|
The goal of this is to be optional, opt-in, and per-model, only for external calls. So you can still have the same flow in the webui chat if you'd like; you could still generate an image and call /images/upscale if you'd like; but you can also now use upscaling for chat apps that don't call /images/upscale (most of em). Given that this project is targeting self-hosters I think this makes a lot of sense, there is no case where the latency is unexplained because they set it up. People with true multi-step image workflows will be using comfyui or something anyway, this directly targets the main usecase for this endpoint. Giving users the choice to get higher quality images in generic chat apps without the exponential cost of generating the base at a higher resolution (especially with consumer hardware).
Next Steps:
|
|
Thanks @NineBallo and @bconsolvo — good discussion. Here's where I'd like to land this. Direction: accept the per-model opt-in, with conditions. The gap is real — OpenAI-compatible clients only call
Process items before this is approvable:
Happy to re-review once those are in. |
892fc44 to
9c72504
Compare
9c72504 to
33ff3a1
Compare
|
@ramkrishna2910 Fixed some stuff up, if you could take another peek it'd be appreciated :) Two notes:
|
ramkrishna2910
left a comment
There was a problem hiding this comment.
Thanks for the rework @NineBallo — I went back through my August 3 list item by item, and substantively all six are met. Recording that explicitly so it doesn't get relitigated:
/images/upscalestays the primitive, both paths sharedo_upscale()— done.- Per-request override — done via
skip_upscale. It's opt-out only rather than the two-way override I described, which I'm fine with: opting in per request without a configured model has no sensible target. - Response transparency — implemented (
upscaled/width/height), with one defect below. - Error handling — done properly.
do_upscale()returnsstd::optionalinstead of inferring fromres.status, which sidesteps the vendored cpp-httplib-1initialization entirely, and/images/upscalekeeps its "ESRGAN upscale failed" 500 via the non-nullrespath. - Docs line on the original being unrecoverable — present, verbatim, in the
upscale_modelrow. --backend— dropped, as asked.
Process items: GUI changes are out, and the doc gaps are closed — upscale_model is in the /v1/load table in docs/api/lemonade.md, skip_upscale is documented on all three endpoints, and there's a new response-format section. The documentation on this PR is genuinely good.
Three things still blocking.
1. CI has never run on this branch.
There are zero check runs against the head commit. This is a cross-repo PR, so the workflows need a maintainer to approve them — I'm kicking that off now, so this one isn't on you. Flagging it because nothing here has been verified by CI yet, and the branch is also 3 commits behind main and will need an update before it can merge.
2. The upscaled flag is coupled to PNG header parsing.
In apply_upscale_if_configured, the image is replaced first, then:
item["b64_json"] = upscaled.value();
std::string raw_header = utils::JsonUtils::base64_decode(upscaled.value().substr(0, 32));
if (auto dims = lemon::utils::get_png_dimensions(raw_header)) {
item["upscaled"] = true;
item["width"] = std::get<0>(*dims);
item["height"] = std::get<1>(*dims);
}upscaled is only set when get_png_dimensions succeeds — but the substitution already happened unconditionally. Any output that isn't a PNG with a well-formed IHDR (or a truncated first chunk) yields an upscaled image carrying no signal at all, which is exactly the "surprising 2048² from a 512² request with no way to detect it" case that transparency requirement existed to prevent. Your own docs make it load-bearing: "Callers should check this field to determine actual image dimensions."
This is latent rather than live today, since both upscaler CLIs emit PNG — but it's a one-line fix and the contract is already written down. Set upscaled unconditionally when the image is replaced, and attach width/height only when the header parses.
3. do_upscale dispatches on the label alone, which breaks once #3327 lands.
The gate is has_label(info.labels, "upscaling"), after which the code hardcodes try_get_spec_for_recipe("sd-cpp") and calls SDServer::upscale_via_cli. Nothing checks the model's actual recipe.
#3327 (in review now) adds five upscalers with recipe: "thenoise", all carrying the upscaling label and all marked suggested: true. Once both land, a user setting upscale_model: "LSDIR-4x-TheNoise" passes your label check, and a TheNoise .safetensors gets handed to sd-cli. It fails — and on the auto path the failure is swallowed by design, so the caller gets an un-upscaled image, no upscaled field, and only a server-side WARNING. Silent wrong behavior from a setting the UI actively suggests.
Your docs already state the requirement ("must be an sd-cpp recipe model with the upscaling label"); the code just doesn't enforce it. #3327 adds recipe dispatch to handle_image_upscale for precisely this reason, so I'd like that PR to land first and this one to route do_upscale through the same dispatch — which turns this from "add a recipe guard" into "reuse what's already there," and gets TheNoise upscalers working on the auto path for free. If #3327 slips, a minimal recipe check with a clear error is fine as a stopgap.
One housekeeping item: the PR description is now stale. It lists ModelOptionsModal.tsx, recipeOptionsConfig.ts, and sdcpp_server.cpp in its file table, none of which are still in the diff, and it doesn't mention the new image_sniff.h. Since the description becomes the squashed commit message, please refresh it to match what actually ships.
Happy to re-review as soon as CI is green and 2 and 3 are addressed — the substance is close.
|
to better coordinate, I'd like add a few more notes about upscaling with TheNoise, since this could be relevant to this PR, especially in trying to keep naming/behavior consistent (or at least non-conflicting) between backends. With #3327 the same mechanism used by sdcpp (cli-based upscaling) will be available (as already mentioned). Additionally, TheNoise already supports (also through lemonade since request parameters are passed down as-is) the generate-then-upscale flow without any additional steps/calls, just by passing specific parameters. This breaks down as:
|
|
@bitgamma Current idea is:
This does leave a gap where: upscale on theNoise paired with a pixel upscale chains; whereas the sd side wont. Given they are on a unified api I think we should figure something out. IMO an ideal solution would be: upscale -> used a a global request level toggle to either skip all upscaling steps (latent or pixel) or hit a default pixel upscaler (2x esrgan). It will have worse quality on theNoise's side but then they are at least unified. refine/prescale/preupscale/latent-upscale -> used as a "theNoise" specific path to hit the latent upscaler first. pixel_upscaler -> model picker, so any request can hit any valid upscale model on either backend. |
|
@NineBallo maybe we can simplify it further:
either of these options can be set independent of each other, so the "stacking" becomes an explicit user choice and not cause a mismatch between the two. In addition, a WDYT? |
|
@bitgamma Only thing is skip_implicit_upscaling is real long, and would skip implicit and explicit upscaling anyways. Thinking we keep the 'skip_upscaling' naming already on this branch since that cleared already. Any objections or can I send it like this? |
|
@NineBallo I won't strongly oppose to the naming, but technically with TheNoise you can send all upscale parameters directly in the request (this doesn't need to change and works even today since parameters are passed through as-is) so 'skip_upscaler' is a bit of a misnomer (but something I can live with). Maybe 'no_implict_upscaler' is a tiny bit shorter? I'll leave this up to you though, as long as functionality matches I am fine with it. |
|
Last ping for now I promise haha Since this would strip all upscaling on the sd side, would it make sense to just catch&drop any upscaling params when it's set for theNoise? Then we match behavior on both. Only concern is maintenance burden depending on how many future params theNoise is planning to add. |
|
@NineBallo no, let's not strip params. That's why I wanted to add "implicit" to the name, because all explicit parameters need to be left alone. The behavior still matches on both, no-Lemonade-driven upscale, but what the backend does internally is not our concern |
4e40337 to
71dccfc
Compare
…ponse transparency - Strip ModelOptionsModal/recipeOptionsConfig GUI changes (defer to GUI3) - Add upscale_model to /v1/load parameter table (docs/api/lemonade.md) - Add upscale_model to image_defaults example (docs/guide/configuration/custom-models.md) - Add skip_upscale request param to /images/generations, /images/edits, /images/variations - Add response transparency: upscaled/width/height fields when auto-upscale fires - Add get_png_dimensions() helper to image_sniff.h - Fix JSON initializer brace mismatches in do_upscale error responses - Restore correct OpenAI error shape in /images/upscale (error body was emitted as an array by the compact json initializer refactor) - Return the original image when auto-upscale fails instead of failing the whole generation request - Upscale every image in the response (n > 1), not just data[0] - Require the upscale model to carry the 'upscaling' label - Reuse JsonUtils::base64_decode for the PNG header instead of a hand-rolled decoder - Make parse_bool_form_field a shared member and fix std::tolower UB - Regenerate docs/dev/backends-reference.md for the new sd-cpp option Accept 'true', 'yes', 'on' (case-insensitive) and '1' as truthy values from multipart form fields instead of only 'true', '1', 'True'. Deduplicate bool form field parsing in handle_image_edits/ handle_image_variations into parse_bool_form_field() static helper.
Catch upscale=bool server-side on generations/edits/variations and route it to the post-generation upscaling step; add thenoise-only refine=bool (forwarded as native upscale=true) and pixel_upscaler on all image endpoints to select any registered upscaler across both backends (alias for model on /images/upscale).
Renames the per-request opt-out on generations/edits/variations to make clear it only suppresses implicit (auto) upscaling, not explicit /images/upscale calls.
The caught upscale flag never altered behavior: the post-generation step is gated by pixel_upscaler / the upscale_model recipe option alone. Also move request param extraction ahead of validation in generations.
71dccfc to
185c52c
Compare
|
@bitgamma |
bitgamma
left a comment
There was a problem hiding this comment.
this doesn't match what I think we agreed on. I made my comments on the documentation items but of course the corresponding code needs to be changed as well.
| | `cfg_scale` | No | Classifier-free guidance scale. SD-Turbo uses low values (~1.0). Default varies by model. | <sub></sub> | | ||
| | `seed` | No | Random seed for reproducibility. If not specified, a random seed is used. | <sub></sub> | | ||
| | `skip_implicit_upscaling` | No | Boolean. If `true`, skip any auto-upscale (skip any auto-upscale configured via `pixel_upscaler`). Defaults to `false`. Replaces the legacy `skip_upscale` field. | <sub></sub> | | ||
| | `upscale` | No | Boolean. Accepted for backward compatibility but not forwarded to the backend. The post-generation upscaling step is controlled by `pixel_upscaler`. | <sub></sub> | |
There was a problem hiding this comment.
refine and pixel_upscaler should only be at recipe options, not per-request. At least that's how I understood it. The entire goal was to enable upscaling for openai-compatible endpoints that aren't aware of these options, wasn't it?
the way you implemented it intentionally breaks clients that want to take advantage of TheNoise's embedded handling for no reason at all.
upscale should be just passed through instead of just being deleted. It has never been defined before so there is no "backwards compatibility" to speak of
If you really want to to make the pixel_upscaler overridable per-request than please rename it to something else (also in the recipe) so that it doesn't clash with TheNoise naming. refine is fine as it doesn't clash
There was a problem hiding this comment.
Yeah that's fair.
A better solution imo is to just pass through the option in the initial generation when TheNoise is used for both gen/upscaling. Though for now I'll go back to the old upscale_model param I was previously using. Trying to unify the api's this much is out of scope and I want to get the bulk of this cleared.
EDIT: Having two identical api params is bugging me, sketching up the solution that preserves both here.
|
@bitgamma Do you hate this logic for pixel_upscaler (sd.cpp@5267)? If so I'll just revert to having upscaling_model and pixel_upscaler but it seems redundant since one is a subset of the other. Idea is that for some settings if we don't have a known recipe for another backend then it falls through to the requested one. This ideally gives us the best of both worlds. (this has been rather loosely tested, want to check if you like the direction before I do a full sweep) |
|
@NineBallo this is even less unified, because in one case (sdcpp) the pixel_upscaler needs to be a model configured in lemonade, while for TheNoise it needs to be an upscaler that the user put in thenoise.upscaler_dir. I think this is too confusing. The only thing that needed to be done for the API to be unified was to keep the original logic for both sdcpp and thenoise, just with the old Basically, from the beginning, my only ask was NOT to use field names used by thenoise (to avoid conflict) and to apply the same logic to both backends since they now have a rather unified dispatch. The |
Summary
Add automatic post-generation upscaling to the
/v1/images/generations,/v1/images/edits, and/v1/images/variationsendpoints. When a model has apixel_upscalerrecipe option (or the caller passespixel_upscalerper-request), the generated image is automatically upscaled before returning. Bothsd-cpp(Real-ESRGAN) andthenoiseupscalers are supported — the correct backend CLI is selected from the upscaler model's recipe.This also introduces
refine(latent-space 2x refine forthenoiseduring generation) andskip_implicit_upscaling(per-request opt-out of auto-upscale). The/v1/images/upscaleendpoint gains apixel_upscaleralias for themodelfield. (just to keep things consistent)Scope
Testing
Testing details:
pixel_upscalerset on the model recipeskip_implicit_upscaling=truebypasses auto-upscalerefine=trueforwardsupscale=truetothenoisebackends onlypixel_upscalerper-request override works for all three image endpoints/v1/images/upscaleacceptspixel_upscaleras alias formodelupscaled,width,heightfields when auto-upscale occursDocumentation
Breaking Changes
AI-assisted contribution
Please select one:
If AI tools were used: