AUTOMATIC1111
Remove scale loop
Proposed actual fix for https://github.com/AUTOMATIC1111/stable-diffusion-webui/issues/4168 and https://github.com/AUTOMATIC1111/stable-diffusion-webui/issues/4211.
I can't find the logic why there is a loop upscaling images 3 times. This PR removes the loop and just runs the image through the selected model, which should be a normal behavior.
@d8ahazard if you know what is the reason for this loop, let me know what it is to try to find a better solution.
Presumably because some of the upscalers upscale by a fixed multiplier and a single pass isn't going to get you to your requested size if it's larger than that multiplier.
It breaks out of the loop once the requested size is reached.
If that's the case, it's probably better to let the users manually (and knowingly) upscale the images as many times as necessary to reach their desired size if the model doesn't reach the required size.
The loop breaks many parts of the logic, such as the 1x models, the tiling to fit limited VRAM (ie. tiles on the second loop can be over the VRAM limit, breaking the logic of the tiles), besides if it does happen to apply the models twice, (or thrice), the ESRGAN models and other upscalers have not been trained in most cases to deal with the "degradation" of images upscaled by the model itself, so it will very probably introduce issues to the images, such as over-sharpening and color shifts.
The ideal solution would be to change the maximum scale of the upscale to whatever the maximum native factor of the models is, but I don't know how to do that in the way the code is structured now.
Users can do that by setting the scale factor to less than or equal to the model's trained resizing, one could argue that the comparison is perhaps off in the break condition if img.width > dest_w and img.height > dest_h: and >= makes more sense as whole number multiples are the default factor and will avoid a whole extra unneeded round of resizes, or that having the wrapped resize models broadcast their native resize factor and have calling code act on that would be useful.
But flat out deleting the loop and leaving the UI and calling code with an unexpectedly small return image is not the solution you want.
The users don't really know what the models scale factor is, since the loop hides this information from them and the final result only states the selected scale, not the real scale. That is also not the solution you want, hiding what it's doing.
The loop is a hack and it causes other things to behave weirdly. The first thing I need to know is if there is no other weird hack somewhere else that depends on this hack and if it can be removed safely before trying to modify the logic, which is why it's set as draft so people can test it.
Here is a compromise, let the user decide with an option in the ui if they want to loop the same model multiple times until it reaches the requested scale above the model's native scale and then downscale to the correct size with the traditional algorithm (lanczos) or disable the loop, scale at the model's native scale and it will reach the requested scale by the traditional scaling algorithm, but 1x models will work without having to change the image scale.
Just looked at your other linked PR, is your motivation here that the 3 times loop is repeating on the 1x networks?
If that's the case then why not check the input and output shapes either side of the upscale, the 1x networks break the upscaling assumption so are never going to benefit from an additional loop:
def upscale(self, img: PIL.Image, scale: int, selected_model: str = None):
self.scale = scale
dest_w = img.width * scale
dest_h = img.height * scale
for i in range(3):
#Revert to GTEQ comparison
if img.width >= dest_w and img.height >= dest_h:
break
init_shape = (img.width, img.height)
img = self.do_upscale(img, selected_model)
#Break early if the network is doing no size change
if (img.width, img.height) == init_shape:
break
if img.width != dest_w or img.height != dest_h:
img = img.resize((int(dest_w), int(dest_h)), resample=LANCZOS)
return img
That in combination with the comparison check change would seem resolve both over-iterations mentioned in that other PR discussion.
@dfaker that won't work, since in the first if condition with the >=, the condition is true already for 1x networks and you break the loop, never applying the model.
Which is the reason why the =, has to removed there from the >=.
You see now why the hack is problematic? You don't understand what it's doing. But yeah, a separate path for 1x models should do it, I'll send another commit when I can connect to the pc.
it should be possible to break early if the dimensions of upscaled image are no different from dimension of original image
also not a hack
There it is, now the 1x scale is separate and applies only once and upscales will go through the loop
Apologies, I didn't realise you'd introduced that bug yourself in #3976!
Everything will need to be 'upscaled' or at least filtered once, due to it being a size increase or them wanting the filtering effect of a 1x network so one pass is guaranteed, I think allowing the two break conditions to be checked after the first upscale round gets us there without UI extension. Retaining the init_shape check if 1x upscalers are run with a scale factor greater than 1
def upscale(self, img: PIL.Image, scale: int, selected_model: str = None):
self.scale = scale
dest_w = img.width * scale
dest_h = img.height * scale
for i in range(3):
init_shape = (img.width, img.height)
img = self.do_upscale(img, selected_model)
if img.width >= dest_w and img.height >= dest_h:
break
if (img.width, img.height) == init_shape:
break
if img.width != dest_w or img.height != dest_h:
img = img.resize((int(dest_w), int(dest_h)), resample=LANCZOS)
return img
init_shape should be defined before the loop, but there, updated the loop with the 2 breaks from your snippet
Looks good, do we want to retain the img = img.resize((int(dest_w), int(dest_h)), resample=LANCZOS) pass even if upscale_loop=False is set? Seems to spoil the purity of getting a direct one pass output if you're potentially going to upscale it with lanczos.
That's a very good point. I would remove the final lanczos resize if not using the loop, but I'm a bit apprehensive as it may introduce another error somewhere else, so I'd rather test it offline longer before suggesting removing it.
I think the only real danger is scale-to
getting too small an image and not being able to crop it as requested.
Yep, exactly part of what I want to test more, as well as SD upscale. I may be able to test during the weekend, but for now I mainly wanted to submit the fix for the slow upscalings
So the user has an option to avoid the loop if the repeated use of the same model causes the known issues like over-sharpening and color shift that come with it
The loop is there for when you use a 2x model to upscale to 3x. You avoid the loop by specifying the right parameters.
When is the user made aware of this and that he may be using wrong parameters?
He is aware because he is using a 2x model to upscale to 3x. If he is not aware a checkbox with loop written on is going to confuse him even more. I'll just make a commit with the fix.