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

High quality Stable Cascade Stage C previews via previewer.safetensors #5035

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

asagi4
Copy link
Contributor

@asagi4 asagi4 commented Sep 23, 2024

This enables higher-quality previews for Stable Cascade.

It seems to be reasonably fast, too; during my testing speed drops from about 4.2 it/s to ~4 it/s, and you can actually see what it's generating unlike the pixely mess that the latent2rgb method provides.

I don't really like that the previewer filename is used to detect which method to use, but it fits with what the code is currently doing anyway.

The mini-decoder preview method option should probably be generalized somehow rather than calling it "taesd"

@asagi4
Copy link
Contributor Author

asagi4 commented Sep 23, 2024

I suppose I could generalize the mechanism so that the latent format class knows how to load its own previewer. I think that would avoid some of the awkwardness of the code.

@comfyanonymous
Copy link
Owner

Yeah having an enum or something on the latent format to set the type of the previewer would be good I think.

@asagi4
Copy link
Contributor Author

asagi4 commented Sep 24, 2024

I was thinking it could just be a "load_preview_decoder" method on the latent format class. That way other code doesn't need to know or care about the filename or the model class.

I'll cook up something later today.

@comfyanonymous
Copy link
Owner

yeah that should also be good.

@asagi4
Copy link
Contributor Author

asagi4 commented Sep 24, 2024

I did a rather minimal refactor to get started.

Notably, the Impact Pack extension broke when I attempted to rename the TAESDPreviewerImpl class into something more generic.

I also added the optional method argument to get_previewer since custom nodes might want to use it themselves without being forced to obey the command line argument (I think that may be why Impact has its own implementation)

@ltdrdata
Copy link
Collaborator

I did a rather minimal refactor to get started.

Notably, the Impact Pack extension broke when I attempted to rename the TAESDPreviewerImpl class into something more generic.

I also added the optional method argument to get_previewer since custom nodes might want to use it themselves without being forced to obey the command line argument (I think that may be why Impact has its own implementation)

You don't need to worry about that. If refactoring is applied, it will be reflected.
All that's needed is a grace period once the merge is decided

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.

3 participants