web ui: display in-progress images - #181
Conversation
lstein
left a comment
There was a problem hiding this comment.
Looks good. Will need some conflict resolution with the recent changes to support streaming of GFPGAN/ESRGAN, but doesn't look too bad. @bakkot drop a comment if you will be doing additional pushes on the branch. Otherwise I can work on it later this afternoon.
654b65b to
c7c7737
Compare
|
@lstein Rebased. |
|
I am only seeing the very first noisy image and then the final output. It's not streaming every 5th image. Am I missing something here? |
| <button type="button" id="reset-all">Reset to Defaults</button> | ||
| <br> | ||
| <label for="progress_images">Display in-progress images (slows down generation):</label> | ||
| <input type="checkbox" name="progress_images" id="progress_images"> |
There was a problem hiding this comment.
Quick consistency nitpick, can this be named progress-images?
There was a problem hiding this comment.
Sure. The underscore was just for consistency with gfpgan_strength immediately below.
There was a problem hiding this comment.
Actually, this file seems to consistently use underscores for names, so I've left it as-is for now.
There was a problem hiding this comment.
@TesseractCat are there still issues, or are we good to go? (I've re-requested a review. Hope that's etiquette.)
There was a problem hiding this comment.
@lstein could this get merged as-is, and then @TesseractCat can submit any further style nits in a followup PR? I'd like to stop needing to rebase this PR, and to get #182 in.
| let progressImageEle = document.querySelector('#progress-image'); | ||
| progressImageEle.src = BLANK_IMAGE_URL; | ||
|
|
||
| progressImageEle.style.display = {}.hasOwnProperty.call(formData, 'progress_images') ? 'initial': 'none'; |
There was a problem hiding this comment.
Why not just do formData.hasOwnProperty?
There was a problem hiding this comment.
You should never do foo.hasOwnProperty. That will break if foo has an own property named hasOwnProperty, or if it has a null prototype. In this particular instance we know it won't, but this is still the idiomatic way to write this test.
| @@ -102,9 +103,20 @@ def image_done(image, seed, upscaled=False): | |||
| {'event':action,'processed_file_cnt':f'{x}/{iterations}'} | |||
There was a problem hiding this comment.
Probably should be processed_file_count, also since this is intended to be consumed by JS, maybe this should be processedFileCount.
There was a problem hiding this comment.
That's not related to this PR.
Nice catch, seems like something broke in a rebase (or PngWriter changed). Stand by... |
|
Ah, it's these lines (I think). I don't know what those are supposed to be doing. @blessedcoolant what was the point of that change? Could it be accomplished in a different way? |
That is to overwrite the existing image when upscaled if the user does not want 2 images. The stops the unique file name from creating a new file name which inadvertently overwrites the previous file only giving a scaled output. The streaming is working here though. #182 |
|
Ah, I was wrong, it was this change, which fails to update the I've pushed a fix for that bug. @blessedcoolant could you see if it works for you now? And @lstein can you confirm that bakkot@d6868bc is a correct change? |
|
Still broken for me. The intermediate frames are being overwritten so the same file name in the folder. In the other PR, they were being written to different files. |
|
@blessedcoolant Hm, did you pull? With the most recent commit, I am again getting files written to different names instead of overwriting. (The other PR is only working because it hasn't been rebased onto the problematic change.) |
Sorry. That was my bad. I thought I pulled. It's working. |
blessedcoolant
left a comment
There was a problem hiding this comment.
Does what the PR says it does for this current point of time.
|
Ping @lstein - this is ready to go. |
|
Sorry about all the confusion regarding pngwriter and duplicate files. There was some pretty nasty stuff that happened when we moved from filenames to streaming Images in the WebUI which broke how the post-processing through GFPGAN and ESRGAN worked. It took me a couple hours to figure out what was going on here, but I cleaned up some code and think it is easier to follow now. Suggestions for further simplication most welcome. |
On its way - just checking edge cases. |
|
The PR is now in main. This is a fun feature! However, during this morning's testing I did notice one non-blocking issue, which is that when an init_img is provided, the in-progress images are not shown. Instead there is a blank area. Also, we need to have a user interface element to clear the init_img field in the WebUI. Right now there's no obvious way to do it, though it is possible by resetting everything or opening the file select dialog and then cancelling. |
Based on #180. Merge that PR first even if you intend to accept this one. Only look at the second commit; the first commit is in #180.This adds an off-by-default checkbox which lets you see images as they render, at the cost of slowing down generation. Doing this on every step ~doubles the time-to-generate on my machine, so I've made it do every 5th image, which seemed like a good tradeoff.