Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Upscale by and Upscaler options to img2img #7931

Merged
merged 5 commits into from Mar 28, 2023

Conversation

space-nuko
Copy link
Contributor

@space-nuko space-nuko commented Feb 19, 2023

Describe what this pull request is trying to achieve.

Adds "upscale by" and "upscaler" options to img2img, with the same functionality as Hires. fix

Closes #7422

Additional notes and description of your changes

"Upscale by" is disabled by default, it only takes effect if the slider's value is > 1. Otherwise the old behavior is used

Infotext and XYZ Grid support was also added

The "Just Resize (latent upscale)" resize mode was removed since it's now redundant

Non-latent upscalers are run before sampling, latent upscalers are run later during sampling

Sadly performance is bad because the event callbacks are run every time the sliders update and not when they're released, this is the corresponding gradio issue: gradio-app/gradio#3204 No longer an issue since webui uses Gradio 3.23.0, can use their .release() callback now

Environment this was tested in

List the environment you have developed / tested this on. As per the contributing page, changes should be able to work on Windows out of the box.

  • OS: Windows
  • Browser: Chrome
  • Graphics card: NVIDIA RTX 3090

Screenshots or videos of your changes

2023-02-19 03_49_47-Stable Diffusion - Chromium

@space-nuko space-nuko changed the title Img2img enhance Add Upscale by and Upscaler options to img2img Feb 19, 2023
@DejitaruJin
Copy link
Contributor

Oh man, I just noticed yesterday that this didn't exist and wondered why.

@anapnoe
Copy link

anapnoe commented Feb 22, 2023

Sadly performance is bad because the event callbacks are run every time the sliders update and not when they're released, this is the corresponding gradio issue: gradio-app/gradio#3204

lets get this approved and I have some ideas how to fix it the trick I am thinking is to create a hidden field and a dummy slider then with JavaScript you can attach an on change event listener on the dummy slider and dispatch the event passing it to the dummy field to update the value (the dom range change event is different from gradios custom event change, it dispatches the value when you release the slider not when you drag it)

@leppie
Copy link
Contributor

leppie commented Feb 23, 2023

I like this! The layout is bit weird on ultrawide screen.

Can you maybe move Upscaler and Upscale by next to one another like in the image below?

image

@AUTOMATIC1111
Copy link
Owner

If the performance is bad then let's not merge this in. If the bad performance us caused by sending in the image for resize from/to text, then let's just ditch the text - it was needed for hr upscale because people were confused, it's not strictly needed here.

As for upscaler selection, what will happen if you choose latent upscale, corp and resize option, 512x512 resolution and a 1024x512 source image?

@anapnoe
Copy link

anapnoe commented Mar 11, 2023

If the performance is bad then let's not merge this in. If the bad performance us caused by sending in the image for resize from/to text, then let's just ditch the text - it was needed for hr upscale because people were confused, it's not strictly needed here.

As for upscaler selection, what will happen if you choose latent upscale, corp and resize option, 512x512 resolution and a 1024x512 source image?

I think the issue with the gradio controls that dispatch only input events are causing performance issues everywhere and are not specific to webui in the ui e.g quicksettings, hires.fix when you upscale by, you can notice the flashing, so many requests just to return the calculated size. I manage to implemented a workaround for all the number fields, and range sliders of the ui to dispatch only on change of the event we need a more solid approach for all the interactive components unfortunately gradio close this like it was a small think it is not only the sliders that are affected but all the input fields as well they need to change and rethink about the architecture of their components until then if we want better components we need to hack them around
space-nuko and AUTO have a look at the UI/UX to see if it suffers the same issue #7519

@space-nuko
Copy link
Contributor Author

space-nuko commented Mar 11, 2023

If the performance is bad then let's not merge this in.

The bad performance is caused by gradio, specifically the slider change event running on every mouse movement. If you could bump gradio's version past 3.20.0 then it could be fixed
https://github.com/gradio-app/gradio/blob/main/CHANGELOG.md#release-event-for-slider
Not to mention, I myself have been frustrated at this slider behavior when adjusting quicksettings since it releases the mouse focus on every update, would be nice to fix that as well

@anapnoe
Copy link

anapnoe commented Mar 11, 2023

If the performance is bad then let's not merge this in.

The bad performance is caused by gradio, specifically the slider change event running on every mouse movement. If you could bump gradio's version past 3.20.0 then it could be fixed https://github.com/gradio-app/gradio/blob/main/CHANGELOG.md#release-event-for-slider Not to mention, I myself have been frustrated at this slider behavior when adjusting quicksettings since it releases the mouse focus on every update, would be nice to fix that as well

gradio only fixed the slider drag-release, this is lame the issue is present on every field if you use the input arrows from the slider to adjust the value same issue there even if bump gradio's version just checked it this fix was a joke i will reopen the issue there somebody has to explain them that this is an important issue affecting all applications
also want to thank you personally for the UniPC sampler I am so happy that it was finally merged great work space-nuko.

@space-nuko
Copy link
Contributor Author

space-nuko commented Mar 11, 2023

Oh I see, filed an issue here gradio-app/gradio#3443

And you're welcome!

@AUTOMATIC1111
Copy link
Owner

What will happen if you choose latent upscale, corp and resize option, 512x512 resolution and a 1024x512 source image?

@space-nuko
Copy link
Contributor Author

@AUTOMATIC1111

Before:
44402-1291363421-masterpiece, solo, 1girl, yellow eyes, green hair, quad braids, very short hair

After:
01894-3082218329-masterpiece, solo, 1girl, grey eyes, brown hair, low-braided long hair, big hair

@space-nuko
Copy link
Contributor Author

Ran across an issue when copying the result image to other img2img tabs, appears to be this Gradio bug: gradio-app/gradio#3623

@SnowingIce
Copy link

Can't wait this PR to be merged!
Highres.fix on img2img would be help a lot in my workflow on converting multiple images to different styles.

@SnowingIce
Copy link

I just tried to incorporate this PR into my local version, which is the stable version 2 weeks ago. It seems that some libraries like Releaseable in gradio are not available.
Since it may affect other PRs, hopefully this PR is not rejected, as it would really be quite useful :(

@space-nuko
Copy link
Contributor Author

Releasable requires Gradio 3.23 since it's a new feature

@AUTOMATIC1111 AUTOMATIC1111 merged commit 4268759 into AUTOMATIC1111:master Mar 28, 2023
2 checks passed
AUTOMATIC1111 added a commit that referenced this pull request Mar 28, 2023
This reverts commit 4268759, reversing
changes made to 1b63afb.
@AUTOMATIC1111
Copy link
Owner

I jumped the gun again and merged this without much thought. I'm reverting it but unfortunately there's no way for me to un-mark this PR as merged.

Here's my list of grievances with it:

  • I don't like how the additions to UI look, and where the upscaler selection is. It should be near where you select resolution. Placing width/height on same row also not very neat.
  • The resize from/to mis-aligns the UI creating empty space as seen on leppie's screenshot.
  • Latent upscale option does not work. It says it didn't didn't find the upscaler and just uses uses normal upscaler.
  • I don't want to have resize from/to in the first place: it's needed in HR upscale where things are complex, for img2img there's no need.

@space-nuko
Copy link
Contributor Author

space-nuko commented Mar 28, 2023

I don't want to have resize from/to in the first place: it's needed in HR upscale where things are complex, for img2img there's no need.

Disagree, I've found it useful almost every time I've used it, I use the output resolution as a mental note of how much memory the operation will take, I don't see why not have an equivalent in img2img, it's the same featureset as HR fix but on a different tab. With an "upscale by" slider I'm always going to want to do the mental calculation of the output resolution anyway

About the rest, I'd be willing to fix the UI to be more to your preferences, although I use a large screen and don't have issues.

@space-nuko
Copy link
Contributor Author

The "resize to" text can be hidden in the case of Upscale By not being used, in that case it's redundant info

@SnowingIce
Copy link

SnowingIce commented Mar 29, 2023

Thanks for the work and I have just tested it out, here I want to share my humble opinion and suggestion:

  1. About the UI I think there needs to be a consistency between txt2img and img2img. It might cause a bit confusion if they are different. I think both looks good (though probably users need some time to adapt to the new one), just that there needs to be a consitency.

txt2img
image

img2img
image

  1. In terms of functionality, it seems they are also a bit different. Given an image of size X*Y, upscale by Z, the new feature in img2img directly processes the image to XZ*YZ, while the Hires.fix is post processing a few steps to upscale generated image of size X*Y, to XZ*YZ. Maybe the difference is by design, but the latter would be really useful to have.

  2. It seems not yet compatible with ControlNet, from my testing. With ControlNet enabled, it is not working.

@space-nuko
Copy link
Contributor Author

space-nuko commented Mar 29, 2023

  1. Personally I like having the "Upscale By" slider on its own row since you can only use either upscale or the width/height sliders, but to each their own. So should width/height each get their own row instead of being put on the same row instead? Or how else would help consistency/UX?
  2. It's by design, the nature of HR fix means there are technically two images generated, one pre-HR and one post-HR. With img2img you already have the pre-HR image, so it acts like the second step of HR only. If you check the "save copy of image before HR fix" option you can see the unscaled image generated with HR on, that's basically what you're providing yourself with img2img
  3. Worth taking a look into. However I was using a version of the ControlNet extention at one point (dont know which version) and it seemed to work fine. Might have been because I was using an image with ControlNet already applied though. In what way specifically does it not work?

@SnowingIce
Copy link

SnowingIce commented Mar 29, 2023

Thank you for your reply.
1 and 2. I was expecting something similar to Highres.fix, so specifically I was referring to the consistency between the new function and the original Highres.fix. As you mentioned the design is to implement an upscaler, then probably my suggestion is not valid.

  1. I run the experiments again, it sometimes work with ControlNet and sometimes doesn't. I could not tell exactly how to replicate the issue at this moment, looks like it's only during the first few times of running.

And I observe one improvement can be done on the UI:
image

  • The resize number does not change with changing width x height selection
  • Upscale by 1: Generated image = selected width x height = 512*320
  • But upscale by non-1 value Z: Generated image = Z * (original width x height) = 1536*936
    Again, maybe this is designed to be so, but it's a bit confusing.
    I would recommend to freeze the panel for width/height selection, when upscale is not 1.

One more thing is that the sampling step sometimes deviate from the chosen sampling step. Or maybe this issue is intrinsic to img2img. (Note that in below image, I have chosen sampling step = 20, but it ran total 16 steps.)

image

Not meaning to be picky, I just work as an avid tester. Really appreciate your work.

@space-nuko
Copy link
Contributor Author

Thanks for the feedback. I think auto's working on some improvements I'll test out shortly

The saga continues in #9108...

Rewinged added a commit to Rewinged/stable-diffusion-webui that referenced this pull request Apr 11, 2023
Rewinged added a commit to Rewinged/stable-diffusion-webui that referenced this pull request Apr 11, 2023
…-enhance"

This reverts commit 4268759, reversing
changes made to 1b63afb.
Rewinged added a commit to Rewinged/stable-diffusion-webui that referenced this pull request Apr 11, 2023
Rewinged added a commit to Rewinged/stable-diffusion-webui that referenced this pull request Apr 11, 2023
…ace-nuko/img2img-enhance"""

This reverts commit 760a7fe.
Rewinged added a commit to Rewinged/stable-diffusion-webui that referenced this pull request May 10, 2023
…from space-nuko/img2img-enhance""""

This reverts commit c80f7db.
Rewinged added a commit to Rewinged/stable-diffusion-webui that referenced this pull request May 10, 2023
…ace-nuko/img2img-enhance"""

This reverts commit 2b25c28.
Rewinged added a commit to Rewinged/stable-diffusion-webui that referenced this pull request May 10, 2023
Rewinged added a commit to Rewinged/stable-diffusion-webui that referenced this pull request May 10, 2023
…ace-nuko/img2img-enhance"""

This reverts commit 760a7fe.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: img2img UI consistency with new Highres. fix
7 participants