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

Jpg config image extension #1863

Merged
merged 21 commits into from
Feb 26, 2024

Conversation

eddyizm
Copy link
Contributor

@eddyizm eddyizm commented Jan 11, 2024

[Added]
feature: added flag, config and ui update for image extension change)[feature: added flag, config and ui update for image extension toggle.

#1789

image

image
image

@mashb1t mashb1t linked an issue Jan 11, 2024 that may be closed by this pull request
Copy link
Collaborator

@mashb1t mashb1t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the implementation.
The config should only be read on app start and not be adjusted during runtime, as this is a global setting.
For UI elements Fooocus currently uses the AsyncJob object, which contains an array of parameters and which is introducing isolation per user/session. Using your approach (config) leads to User 1 updating the output extension also for User 2, who might not be aware / is not made aware of in the UI that the change happened and this way the feature isn't usable in isolation.

This is why i'd propose to add the UI element to the ctrls in webui.py, add a pop() call in async_worker.py and hand it over to the private_logger.py's log() method as a parameter. The radio buttons should also only be shown/interwctive when history log is enabled (see args).
The config is useful and can still be used as default for the instance.

What do you think?

@eddyizm
Copy link
Contributor Author

eddyizm commented Jan 11, 2024

Hi thanks for the feedback and clarification. Did not realize the config was global so thanks for pointing that out.
Your suggestions seem reasonable and will give it a go this evening. Appreciate the guidance. New to gradio so this seemed like a good entry point to be able to contribute.

@mashb1t mashb1t marked this pull request as draft January 12, 2024 17:00
@RodTDai
Copy link

RodTDai commented Jan 12, 2024

This is awsome guys, thanks for the work you are all doing.

@eddyizm
Copy link
Contributor Author

eddyizm commented Jan 13, 2024

Updated with the changes requested. Please let me know if I missed something.

@eddyizm eddyizm marked this pull request as ready for review January 13, 2024 07:31
@eddyizm eddyizm requested a review from mashb1t January 13, 2024 16:27
Copy link
Collaborator

@mashb1t mashb1t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eddyizm i've fixed a bug (missing extension param), added webp support and adjusted the label, as the label "Image Extension" could be misunderstood as images shown in the webui are still always png (see issue #1421). The wording is still not 100% perfect, feel free to adjust to your liking.

Comparison between output formats, incl. webp (same seed & settings):
image

What do you think, should we integrate the proposed solution of #1421 to this feature?

@eddyizm
Copy link
Contributor Author

eddyizm commented Jan 14, 2024

@mashb1t I agree the naming is not correct. by using image extension we are implying format which it looks like originally I was just changing the naming convention, so more like image naming extension or file extension. When I was changing it I was looking at the gradio docs because there should really be some type of mouse over with a not or a star to say "this does not change the actual creation or compression logic, just simply changes the file extension for convenience" or something to that effect.

Although after seeing the outputs, it does appear to be affecting the size of the file.

Good catch on the log parameter, seems like I had it in there at some point and backed it out.

Everything else looks good to me!

EDIT: Yes, I seen the linked issue. It is also a problem I've been having with Firefox since day one until I stopped using it. But if we can fix it, let's do it. It is quite annoying.

returns and uses filepaths instead of numpy image by saving to temp dir
uses double the temp dir file storage on disk as it saves to temp dir and gradio temp dir when displaying the image, but reuses logged output image
…nfig_image_extension

# Conflicts:
#	modules/async_worker.py
#	modules/private_logger.py
@mashb1t
Copy link
Collaborator

mashb1t commented Jan 14, 2024

@eddyizm functionality from #1932 has been merged, works perfectly for me. Now also the preview / final images in gradio do have the selected file extension.
Maybe we can rename the label / field to "Image File Extension", as this plainly states what it is (both gradio and outputs).

Please check, happy to hear your feedback!

@mashb1t mashb1t linked an issue Jan 14, 2024 that may be closed by this pull request
@eddyizm
Copy link
Contributor Author

eddyizm commented Jan 14, 2024

Looks good to me. That last label change is perfect!

quality=95
optimize=True
progressive=True
@mashb1t
Copy link
Collaborator

mashb1t commented Jan 18, 2024

@eddyizm A label and naming even better describing what it is is "Output Format". Hope this is also ok for you.

@eddyizm
Copy link
Contributor Author

eddyizm commented Jan 18, 2024

Yup makes more sense. I was thinking about changing it as well. Good call.

@mashb1t
Copy link
Collaborator

mashb1t commented Feb 4, 2024

@eddyizm fyi now that #1940 has fully been implemented i've also added EXIF metadata handling for jpg and webp, see mashb1t@b281375

@eddyizm
Copy link
Contributor Author

eddyizm commented Feb 4, 2024

This bad boy keeps getting bigger and bigger!

@mashb1t mashb1t added Size M medium change, isolated, testing with care Size L large change, may depend on other changes, thoroughly test and removed Size M medium change, isolated, testing with care labels Feb 9, 2024
@mashb1t mashb1t added this to the 2.2.0 milestone Feb 24, 2024
@mashb1t mashb1t changed the base branch from main to develop February 26, 2024 13:29
@mashb1t
Copy link
Collaborator

mashb1t commented Feb 26, 2024

@eddyizm would it be ok for you if i add myself as the committing author?
13/18 commits were done my be and I've already integrated EXIF data handling for jpg and webp.
ofc. adding you as co-author

@eddyizm
Copy link
Contributor Author

eddyizm commented Feb 26, 2024

@mashb1t sure. No problem at all!

@mashb1t mashb1t merged commit 15425f8 into lllyasviel:develop Feb 26, 2024
mashb1t added a commit that referenced this pull request Feb 26, 2024
…1863)

* feature: added flag, config and ui update for image extension change #1789

* moved function to config module

* moved image extension to webui via async worker. Passing as parameter to log and get_current_html_path functions per feedback

* check flag before displaying image extension radio button

* disabled if image log flag is passed in

* fix: add missing image_extension parameter to log call

* refactor: change label

* feat: add webp to image_extensions

supported image extemsions: see https://pillow.readthedocs.io/en/stable/handbook/image-file-formats.html

* feat: use consistent file name in gradio

returns and uses filepaths instead of numpy image by saving to temp dir
uses double the temp dir file storage on disk as it saves to temp dir and gradio temp dir when displaying the image, but reuses logged output image

* feat: delete temp images after yielding to gradio

* feat: use args temp path if given

* chore: code cleanup, remove redundant if statement

* feat: always show image_extension element

this is now possible due to image extension support in gradio via #1932

* refactor: rename image_extension to image_file_extension

* feat: use optimized jpg parameters when saving the image

quality=95
optimize=True
progressive=True

* refactor: rename image_file_extension to output_format

* feat: add exif handling

* refactor: code cleanup, remove items from metadata output

---------

Co-authored-by: Manuel Schmid <[email protected]>
Co-authored-by: Manuel Schmid <[email protected]>
Co-authored-by: Manuel Schmid <[email protected]>
Co-authored by: eddyizm <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size L large change, may depend on other changes, thoroughly test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jpeg 100% quality save Image save filename consistency
3 participants