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: Skip the installation of the pillow version that has issues. #3442

Closed

Conversation

ltdrdata
Copy link
Collaborator

handle this issue:

#3416 (comment)

@ltdrdata ltdrdata requested a review from comfyanonymous as a code owner May 11, 2024 02:24
Copy link
Contributor

@shawnington shawnington left a comment

Choose a reason for hiding this comment

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

Indeed there is a regression in 10.3.0 that breaks the fix the worked with 10.2.0 this should be merged immediately.

@bigcat88
Copy link
Contributor

Pillow 10.2.0 has a very serious "bug" with JPEG images having incorrect data: python-pillow/Pillow#7879

I specifically created that issue because photos exported from macOS were causing a crash on our ComfyUI server due to a Pillow error.

@shawnington
Copy link
Contributor

shawnington commented May 11, 2024

Pillow 10.2.0 has a very serious "bug" with JPEG images having incorrect data: python-pillow/Pillow#7879

I specifically created that issue because photos exported from macOS were causing a crash on our ComfyUI server due to a Pillow error.

Do you have a recommended version or any insight as to why 10.2.0 is fine for the android photo, but 10.3.0 is not? Reading on the fix to the error you linked and the error on android, they seem to be tangentially related errors.

It does note in your issue however that ImageFile.LOAD_TRUNCATED_IMAGES does fix the issue in 10.2 and allow the photo to load, we do have that implemented, and the issue is that apparently that solution doesn't work for some photos in 10.3

Can you test with 10.2 and the offending image that caused you to open the Issue? If it works with our current fix of toggling load_truncated_images if an image fails to load, 10.2 should be a safe version number for the time being until the android issues are also resolved.

@bigcat88
Copy link
Contributor

Can you test with 10.2 and the offending image that caused you to open the Issue? If it works with our current fix of toggling load_truncated_images if an image fails to load, 10.2 should be a safe version number for the time being until the android issues are also resolved.

It opens almost fine with ImageFile.LOAD_TRUNCATED_IMAGES (main image decodes successfully)

from PIL import Image, ImageFile

ImageFile.LOAD_TRUNCATED_IMAGES = True
im = Image.open("328384445-d4972d5b-3409-4d5f-a107-ccb8c5dc0177.jpg")
print(im)
im.load()
im.show()

The problem is that this file is not a "JPG", it is MPO file, and Pillow fails to decode the second image.

image

Anyway it is a good idea to create an issue in Pillow.

@bigcat88
Copy link
Contributor

Created one: python-pillow/Pillow#8052

@bigcat88
Copy link
Contributor

As mentioned in issue related to this PR(#3416 (comment)) Pillow 10.3.0 has more correct behaviour then Pillow 10.2.0

I updated my issue in Pillow repo, but that even if they add corrct decoding of second image it want help us in this ComfyUI case, as second image will be anyway smaller in size.

The same will be in case if ComfyUi will support AVIF or HEIC files in future, they can contains multiple images/frames with different resolution.

@shawnington
Copy link
Contributor

shawnington commented May 11, 2024

As mentioned in issue related to this PR(#3416 (comment)) Pillow 10.3.0 has more correct behaviour then Pillow 10.2.0

I updated my issue in Pillow repo, but that even if they add corrct decoding of second image it want help us in this ComfyUI case, as second image will be anyway smaller in size.

The same will be in case if ComfyUi will support AVIF or HEIC files in future, they can contains multiple images/frames with different resolution.

The issue seems to be with this.

if len(output_images) > 1:
    output_image = torch.cat(output_images, dim=0)
    output_mask = torch.cat(output_masks, dim=0)

It's contacting on dim=0 so it's creating a list of photos with the extras that are not going to be used, and failed if the dimensions are different.

Are there any use cases that I am not aware of where Ideal behavior wouldn't simply be to discard the smaller thumbnails so that a single image is returned?

@comfyanonymous
Copy link
Owner

If the images are different sizes I think it makes sense to keep only the ones with the largest size.

@bigcat88
Copy link
Contributor

bigcat88 commented May 12, 2024

Are there any use cases that I am not aware of where Ideal behavior wouldn't simply be to discard the smaller thumbnails so that a single image is returned?

By default, Pillow will always open first the main(primary) image in the list as the first image.

That is, even if there is some kind of wonderful format in the form of “heic/avif” - when you open an image with Pillow, in PIL.ImageSequence primary image will be the first one, even if it is the second one in the file by position.

Discarding all other images will not have big negative effect, as far as I know, usually all interested only in main image(GIF only is an exception).

It is not possible to make this universal in theory, imho.

Scaling second one image to the primary in size will not work for all formats, as for different formats second image can mean different things and be not in proportion(width/height) to first.

It is only for the MPO format the second image is Auxiliary Image to make HDR.
All relatively new formats defines Auxiliary and Depth images separatly.

@shawnington
Copy link
Contributor

If the images are different sizes I think it makes sense to keep only the ones with the largest size.

I just create a pull request with a proposed fix.

@shawnington
Copy link
Contributor

shawnington commented May 12, 2024

Are there any use cases that I am not aware of where Ideal behavior wouldn't simply be to discard the smaller thumbnails so that a single image is returned?

By default, Pillow will always open first the main(primary) image in the list as the first image.

That is, even if there is some kind of wonderful format in the form of “heic/avif” - when you open an image with Pillow, in PIL.ImageSequence primary image will be the first one, even if it is the second one in the file by position.

Discarding all other images will not have big negative effect, as far as I know, usually all interested only in main image(GIF only is an exception).

It is not possible to make this universal in theory, imho.

Scaling second one image to the primary in size will not work for all formats, as for different formats second image can mean different things and be not in proportion(width/height) to first.

It is only for the MPO format the second image is Auxiliary Image to make HDR. All relatively new formats defines Auxiliary and Depth images separatly.

I just proposed a fix that discards additional images if they are smaller. Im not aware of any use cases where additional images are used in comfy, but I didn't take only the first image from the iterator.

Behavior should be the same in 10.2 where the 2nd image is the same size, but should only return one image in 10.3

Id now recommend requirements PIL > 10.2

@ltdrdata
Copy link
Collaborator Author

ltdrdata commented May 12, 2024

I'll close this PR.

@ltdrdata ltdrdata closed this May 12, 2024
@ltdrdata ltdrdata deleted the fix/pillow-10.3.0-issue branch May 12, 2024 00:32
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.

4 participants