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

[13.0][FIX] dms: dms.file thumbnail generation checks for supported mimetypes #236

Merged
merged 1 commit into from
Apr 19, 2023

Conversation

aleuffre
Copy link
Contributor

@aleuffre aleuffre commented Apr 3, 2023

The method _compute_image_1920 of dms_file checks if the file is of mimetype image and tries to use it as a thumbnail. However, there are several types of image files that Odoo/PIL cannot work with, leading to the following error:

Traceback (most recent call last):
  File "/opt/odoo/custom/src/odoo/odoo/tools/image.py", line 404, in base64_to_image
    return Image.open(io.BytesIO(base64.b64decode(base64_source)))
  File "/usr/local/lib/python3.6/site-packages/PIL/Image.py", line 2687, in open
    % (filename if filename else fp))
OSError: cannot identify image file <_io.BytesIO object at 0x7f0a6ecb1780>

and also further down

odoo.exceptions.UserError: ('This file could not be decoded as an image file. Please try with a different file.', '')

Examples of such files are CAD files (mimetype: image/vnd.dwg) For a list of all mimetypes starting with "image/" see: https://www.iana.org/assignments/media-types/media-types.xhtml#image

@aleuffre
Copy link
Contributor Author

aleuffre commented Apr 3, 2023

The pre-commit error seems unrelated to my PR. Unsure how to fix it...

@pedrobaeza
Copy link
Member

I think we should instead restrict the mimetypes to the ones supported.

@pedrobaeza
Copy link
Member

And next time, it's better to only propose the PR for one branch, and wait until the feedback and being merged for not having to modify 3 PRs at the same time (although thank you very much for caring about all the versions 😃)

@aleuffre
Copy link
Contributor Author

aleuffre commented Apr 3, 2023

I think we should instead restrict the mimetypes to the ones supported.

I'll try to find that out and implement it. Thanks for the feedback

@aleuffre
Copy link
Contributor Author

aleuffre commented Apr 4, 2023

Having a fair amount of trouble finding any sort of exhaustive list of mimetypes pillow supports.

I think the best I can do is to start with a conservative list of image types that are for sure supported and then maybe it can be expanded in the future. (I also don't necessarily have all these files using different extensions at hand to easily test the PR with)

@pedrobaeza
Copy link
Member

Ok, then maybe the try/except approach is better having that uncertainty. My only fear was the memory consumption, as for example DWG files are usually very heavy.

@pedrobaeza
Copy link
Member

I'm seeing that you can query the supported mimetypes seeing if included in Image.MIME. That can be other way.

@aleuffre
Copy link
Contributor Author

aleuffre commented Apr 4, 2023

Ok, then maybe the try/except approach is better having that uncertainty. My only fear was the memory consumption, as for example DWG files are usually very heavy.

I agree. Maybe a size limit could work instead? Don't try to generate a thumbnail if the image is bigger than 20MB?

@aleuffre
Copy link
Contributor Author

aleuffre commented Apr 4, 2023

I'm seeing that you can query the supported mimetypes seeing if included in Image.MIME. That can be other way.

I'll check that out.

@aleuffre
Copy link
Contributor Author

aleuffre commented Apr 4, 2023

The list of mimetypes in the registry is surprisingly small. In Odoo shell on v13, python 3.6 and Pillow 5.4.1:

>>> from PIL import Image
>>> Image.MIME
{'BMP': 'image/bmp', 'PNG': 'image/png', 'GIF': 'image/gif', 'TIFF': 'image/tiff', 'JPEG': 'image/jpeg'}

Thanks a lot for the pointers, by the way!

@aleuffre aleuffre changed the title [13.0][FIX] dms: try/except for thumbnail generation of dms.file [13.0][FIX] dms: dms.file thumbnail generation checks for supported mimetypes Apr 4, 2023
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

I see... The advantage of this method is that if PIL evolves for further formats, we will be covered without doing any change in our code.

I would bet that latest versions have support for WEBP and other formats.

@pedrobaeza
Copy link
Member

@victoralmau can you check about pre-commit in other PR?

@pedrobaeza
Copy link
Member

I'm wondering now what happens with SVG files. Have you checked them?

@aleuffre
Copy link
Contributor Author

(Sorry, didn't have time to look at this this week, will pick it back up asap)

@pedrobaeza
Copy link
Member

You can rebase now to have green CI. Please check the SVG part.

@aleuffre
Copy link
Contributor Author

aleuffre commented Apr 19, 2023

I'm wondering now what happens with SVG files. Have you checked them?

I can confirm that if you upload an SVG file, the preview is not automatically generated. However, SVG files do work if they're used manually on the image_1920 field.

Whereas, with a .png file, the preview is automatically set when uploading the file itself.

Clearly, Image.MIME doesn't actually have all the possible mimetypes that Pillow can handle.

@pedrobaeza
Copy link
Member

Well, I think we can start this with such list + SVG manually added to the supported ones.

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

The method _compute_image_1920 of dms_file checks if the file is of
mimetype image and tries to use it as a thumbnail.
However, there are several types of image files that Odoo/PIL
cannot work with, leading to the following error:

```
Traceback (most recent call last):
  File "/opt/odoo/custom/src/odoo/odoo/tools/image.py", line 404, in base64_to_image
    return Image.open(io.BytesIO(base64.b64decode(base64_source)))
  File "/usr/local/lib/python3.6/site-packages/PIL/Image.py", line 2687, in open
    % (filename if filename else fp))
OSError: cannot identify image file <_io.BytesIO object at 0x7f0a6ecb1780>
```

and also further down

```
odoo.exceptions.UserError: ('This file could not be decoded as an image file. Please try with a different file.', '')
```

Examples of such files are CAD files (mimetype: image/vnd.dwg)
For a list of all mimetypes starting with "image/" see:
https://www.iana.org/assignments/media-types/media-types.xhtml#image
@aleuffre
Copy link
Contributor Author

Well, I think we can start this with such list + SVG manually added to the supported ones.

Done. Thank you again for the support with this issue.

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

/ocabot merge patch

Please fw-port it to upper versions.

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 13.0-ocabot-merge-pr-236-by-pedrobaeza-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 9b31da9 into OCA:13.0 Apr 19, 2023
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 3d537d8. Thanks a lot for contributing to OCA. ❤️

@aleuffre aleuffre deleted the 13.0-dms-image-fix branch April 20, 2023 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants