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

fix: 🐛 limit loadImage dropdown to supported formats #4148

Closed
wants to merge 1 commit into from

Conversation

melMass
Copy link
Contributor

@melMass melMass commented Jul 31, 2024

This limits the dropdown of the LoadImage to only display formats it can load:

{'.grib', '.jpx', '.ico', '.jpg', '.bw', '.fli', '.jfif', '.mpg', '.pxr', '.pgm', '.sgi', '
.ras', '.bufr', '.dcx', '.tiff', '.pcx', '.ppm', '.msp', '.jpf', '.pnm', '.h5', '.jpeg', '.
vda', '.cur', '.ftc', '.flc', '.tga', '.im', '.mpo', '.bmp', '.palm', '.xpm', '.pcd', '.iim
', '.j2k', '.jpc', '.ps', '.fit', '.blp', '.tif', '.rgb', '.hdf', '.wmf', '.webp', '.gif', 
'.vst', '.mpeg', '.eps', '.icb', '.jpe', '.jp2', '.dds', '.pdf', '.emf', '.icns', '.fits', 
'.apng', '.gbr', '.rgba', '.dib', '.psd', '.png', '.ftu', '.pbm', '.xbm', '.j2c', '.qoi'} 

This is because extensions like VHS upload videos to input, other audio extensions too and these video/audio get listed in loadImage

@melMass melMass requested a review from comfyanonymous as a code owner July 31, 2024 01:26
@mcmonkey4eva mcmonkey4eva added the Feature A new feature to add to ComfyUI. label Jul 31, 2024
@melMass melMass force-pushed the fix/load-image-only branch 2 times, most recently from 1e198f2 to f1a1659 Compare August 3, 2024 12:45
@Amorano
Copy link

Amorano commented Aug 4, 2024

Perhaps testing for all those formats. Just because it says it supports it, doesnt mean it loads. I have tried

PSD - fail
mpg - fail
tif - fail

I say this because the filter for loading an image into the LoadImage node has restricted filters for the types supported, which is really only jpg, png and webp, so I am unsure it can actually handle loading all those.

IT SHOULD but does not seem to be the case.

@melMass
Copy link
Contributor Author

melMass commented Aug 4, 2024

PSD - fail mpg - fail tif - fail

What do you mean by fail? That the preview doesn't update?

Because this PR is only limiting what is shown in the list of LoadImage.
From the formats you quoted I do use tiff "regularly", you can try this one:

And PIL should load all these but probably don't return the same things, I know that PSD for one is not simple a PIL.Image but a more scoped type

@Amorano
Copy link

Amorano commented Aug 4, 2024

PSD - fail mpg - fail tif - fail

What do you mean by fail? That the preview doesn't update?

Because this PR is only limiting what is shown in the list of LoadImage. From the formats you quoted I do use tiff "regularly", you can try this one:

* [rgb-3c-8b.zip](https://github.com/user-attachments/files/16488575/rgb-3c-8b.zip)

And PIL should load all these but probably don't return the same things, I know that PSD for one is not simple a PIL.Image but a more scoped type

Correct. They dont load in the preview.

And for sure PIL loads them, as I load them using my Queue node, but the LoadImage node does not display the image:

image

It sticks to the previous cached image.

It also doesnt load ALPHA (1-MASK) but this is an ongoing struggle.

P.S. ignore the color change; that was the first network I slapped your test image into -- image is fine, just doesnt preview.

@melMass
Copy link
Contributor Author

melMass commented Aug 4, 2024

Yes, it makes sense actually, the frontend uses the endpoint view

function showImage(name) {
const img = new Image();
img.onload = () => {
node.imgs = [img];
app.graph.setDirtyCanvas(true);
};
let folder_separator = name.lastIndexOf("/");
let subfolder = "";
if (folder_separator > -1) {
subfolder = name.substring(0, folder_separator);
name = name.substring(folder_separator + 1);
}
img.src = api.apiURL(`/view?filename=${encodeURIComponent(name)}&type=input&subfolder=${subfolder}${app.getPreviewFormatParam()}${app.getRandParam()}`);
node.setSizeForImage?.();
}
and creates an image from the URL.
A straightforward solution would be to do an X to png conversion in the endpoint declaration for types not natively supported by browsers. It isn't something you call repeatedly and the cost would only be there for these formats

@christian-byrne
Copy link
Contributor

Perhaps testing for all those formats. Just because it says it supports it, doesnt mean it loads. I have tried

PSD - fail mpg - fail tif - fail

I say this because the filter for loading an image into the LoadImage node has restricted filters for the types supported, which is really only jpg, png and webp, so I am unsure it can actually handle loading all those.

IT SHOULD but does not seem to be the case.

That issue already exists. This PR doesn't add any new formats to the dropdown selection it just filters out non-image formats. A separate issue should be made to discuss browser compatbility with the LoadImage preview.

@christian-byrne
Copy link
Contributor

This is another possible solution which is more performant and can be used for LoadAudio, LoadImage, LoadVideo:

@melMass melMass force-pushed the fix/load-image-only branch from f1a1659 to fb0bd6d Compare August 16, 2024 12:51
@melMass
Copy link
Contributor Author

melMass commented Aug 16, 2024

Closing in favor of #4054 and to avoid bumping it each time I rebase/force push

@melMass melMass closed this Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature A new feature to add to ComfyUI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants