algoo / preview-generator

generates previews of files with cache management
https://pypi.org/project/preview-generator/
MIT License
235 stars 51 forks source link

Why is it that when previewing an image, it is first converted to png and then cropped to jpg with pillow? #271

Closed a523 closed 2 years ago

a523 commented 2 years ago

I have tested, preview the image file (whether it is a jpg file or a png file ), the also call ImagePreviewBuilderIMConvert to convert to png and then call ImagePreviewBuilderPillow ? Why? Why do not use ImagePreviewBuilderPillow directly?

I am use the latest code on the develop branch .

inkhey commented 2 years ago

Hello, ImagePreviewBuilderIMConvert is the builder for "ImageMagick" , so the correct behavior should be converting directly to jpg in ImageMagick builder using Imagemagick command line.

This "convert to png first" behavior is bad, i discovered some time ago, that it has some memory side effect which are not cool: https://github.com/algoo/preview-generator/issues/261#issuecomment-874796756 The reason of using pillow builder here is to have consistent resizing and jpeg result. The good solution should be taking time understanding ImageMagick command line to know how to have similar behavior than in pillow:

That's a point i want to address but i don't have much time for it now. Be free to propose a PR to fix this.

a523 commented 2 years ago

Thank you for your reply, I just now looked at the code of other builders. Other builders like ImagePreviewBuilderDrawio need to convert to image first, and then call Pillow to resize it. So in order to unify the effect of the image, the final use of pillow to unify the adjustment again.

I have a question: Maybe ImageMagick supports more image formats, so you choose ImageMagick as the default builder for images. first use ImageMagick to convert to intermediate format png, and then use pillow to resize

But why not use ImageMagick for the final image resizing (including other builders)? So there is no more problem? and not should take care of having similar quality result between pillow and img builder, the all use ImageMagick.

Are there things that pillow can do that ImageMagick can't?

Yes, I'm interested in this project because I'm using it, let me see and if I can I'll propose a PR.

inkhey commented 2 years ago

But why not use ImageMagick for the final image resizing (including other builders)? So there is no more problem? and not should take care of having similar quality result between pillow and img builder, the all use ImageMagick.

Are there things that pillow can do that ImageMagick can't?

I don't think there is format that pillow can do that imagemagick don't. And if there are, there are not activated in preview-generator.

Yes, I'm interested in this project because I'm using it, let me see and if I can I'll propose a PR.

That's a good idea. I don't really know why there is historically multiple builder for similar format: imagemagick (cli), wand (python lib using imagemagick, deprecated now), pillow. But i agree, we should rationate this part with only one builder.

That's a point i already suggested there: https://github.com/algoo/preview-generator/issues/253 but i proposed wand instead of imagemagick cli, because python library look like a more clean approach. But now, i'm not sure that a good idea.

So i suggest both solution:

I don't have strong opinion on this, so, i suggest to adapt imagemagick command line builder as this is easier and we don't have strong performance need for now requiring using library.

a523 commented 2 years ago

Yeah, if we need to fix this immediately, it might be better to improve ImageMagick command line builder, because is less work. At the moment our project is not particularly urgent, I would probably prefer to find a good ImageMagick lib (it could be wand or others). But that doesn't rule out, by way of the ImageMagick command line, let me see and try.

a523 commented 2 years ago

I found a best practice on producing thumbnails with ImageMagick: https://legacy.imagemagick.org/Usage/thumbnails/

a523 commented 2 years ago

Hi,@inkhey I have proposed a PR to fix this ,please review it.

inkhey commented 2 years ago

fixed by #274