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

Modify Wand builder and replace Imagemagick and Pillow with it #274

Merged
merged 25 commits into from
Nov 18, 2021

Conversation

a523
Copy link
Contributor

@a523 a523 commented Oct 20, 2021

#271
#253

PR Goal

Now the image__wand builder can work as the previous pillow builder, them passed the same test cases. And replace pillow and Imagemagick(command line) builder with wand builder.

Implemented solution

Now the image__wand builder is the default builder for image files,
and also the image__wand builder instead of pillow builder when other builder call
How to modify wand ?
The following information is referenced:
https://legacy.imagemagick.org/Usage/thumbnails/
https://stackoverflow.com/questions/25438402/python-wand-how-to-resize-keeping-aspect-ratio-and-filling-in-the-remaining-spac/25452063#25452063

Checkpoints

  • If relevant, manual tests have been done to ensure the stability of the whole application and that the involved feature works
  • Automated tests covering the feature or the fix, have been written, deemed irrelevant
    (give the reason), or an issue has been created to implement the test (give the link)
tests/input/drawio/test_drawio.py:21 (test_drawio_to_jpeg)
181 != 185

Expected :185
Actual   :181

Only thetest_drawio_to_jpeg test case failed, I think it was the case itself because the pillow builder also failed, if I can confirm, I will raise another PR to fix this test case.

  • The PR or the original issue contains a short summary of the implemented solution.

@a523 a523 changed the title modify Wand builder Modify Wand builder and replace Imagemagick and Pillow with it Oct 20, 2021
@a523
Copy link
Contributor Author

a523 commented Oct 20, 2021

This modified Wand builder can pass the following tests:

tests/input/xcf/test_xcf.py test_to_jpeg fails with wand backend. #114
tests/input/svg/test_svg.py test_to_jpeg fails with wand backend. #115
tests/input/pdf/test_pdf.py test_to_pdf fails with imconvert backend. #116
tests/input/pdf/test_pdf.py test_to_jpeg fails with wand backend. #117
tests/input/txt/test_txt.py test_text_to_jpeg fail with wand and imconvert backend.

Copy link
Contributor

@inkhey inkhey left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution. Nice work, it does work for most content, but there is still missing case .

There's still some behaviour that doesn't work as expected:

  • You need to reintroduce white background to not have weird and poor quality result for file with transparency. You can try with https://fr.wikipedia.org/wiki/Portable_Network_Graphics#/media/Fichier:PNG_transparency_demonstration_1.png , https://commons.wikimedia.org/wiki/File:Psychedelic_Peacesign_Gif.gif , and other (svg and 3d file are concerned too by the issue) . Unfortunately our test are not really effective to check theses case.

  • You should probably deprecate both Pillow and ImageMagick (command line) builder like the Wand one. If you don't do so, it will fallback to them, and i don't think it make much sense if we want to simplify the process by having just one builder.

  • xcf and all raw image file are not supported by wand builder on my own test (disabling the 2 other builders).

  • There is no more quality/progressive/resample_algorithm options like in Pillow. I'm not sure there is a one to one conversion possible but i do think it's possible to have similar config for imagemagick.

Edit: another file to check for transparency: #261

@a523
Copy link
Contributor Author

a523 commented Oct 22, 2021

You need to reintroduce white background to not have weird and poor quality result for file with transparency.

fixed with commit 24463a1 and 1af97cc

You should probably deprecate both Pillow and ImageMagick (command line) builder

fixed with 7572911
But you need to see if replacing pillow with Wand when handlingapplication/postscript files is your expectations. (Just need modify ImageMagick6 policy, then IM can work)

xcf and all raw image file are not supported by wand builder

fixed with 7572911

There is no more quality/progressive/resample_algorithm options like in Pillow.

Yeah, quality/progressive/resample_algorithm there are all in ImageMagick.

I have created progressive JPEG image with -interlace Plane option, 6dbd8d3
But quality and resample_algorithm are used by default value, if we want we can specify a value also.

@lebouquetin
Copy link
Member

You need to reintroduce white background to not have weird and poor quality result for file with transparency.
fixed with commit 24463a1 and 1af97cc

@inkhey we lost several times the white background without to detect it before releasing. Do we have a dedicated test case? It would be interesting to have one in order to ensure quality based on automatic tests instead of developers watchfulness

@a523
Copy link
Contributor Author

a523 commented Oct 22, 2021

You need to reintroduce white background to not have weird and poor quality result for file with transparency.
fixed with commit 24463a1 and 1af97cc

@inkhey we lost several times the white background without to detect it before releasing. Do we have a dedicated test case? It would be interesting to have one in order to ensure quality based on automatic tests instead of developers watchfulness

@lebouquetin
Yes, the test case is already included in the PR, commit :1af97cc

@inkhey inkhey mentioned this pull request Nov 3, 2021
3 tasks
@inkhey inkhey self-requested a review November 18, 2021 14:52
inkhey and others added 6 commits November 18, 2021 23:08
* misc: add concourse ci
* fix: update drawio with functional install link

PR ref: algoo#279
Co-authored-by: raphj <[email protected]>
- Add all office mimetypes mapping supported by LibreOffice to mimetypes_storage to avoid issue of lacking mimetype in system.
- add known issue info about mimetypes issues.
- better logs when mimetype is not found.

Issue ref: algoo#283
PR ref: algoo#284
Co-authored-by: raphj <[email protected]>
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