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

TRUNK-6282: Support uploading webp files as attachments #4807

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wikumChamith
Copy link
Member

@wikumChamith wikumChamith commented Oct 29, 2024

Description of what I changed

javax.imageio does not support webp files by default. Adding org.sejda.imageio:webp-imageio to support webp files.

Issue I worked on

see https://openmrs.atlassian.net/browse/TRUNK-6282

Checklist: I completed these to help reviewers :)

  • My IDE is configured to follow the code style of this project.

    No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend

  • I have added tests to cover my changes. (If you refactored
    existing code that was well tested you do not have to add tests)

    No? -> write tests and add them to this commit git add . && git commit --amend

  • I ran mvn clean package right before creating this pull request and
    added all formatting changes to my commit.

    No? -> execute above command

  • All new and existing tests passed.

    No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.

  • My pull request is based on the latest changes of the master branch.

    No? Unsure? -> execute command git pull --rebase upstream master

@dkayiwa
Copy link
Member

dkayiwa commented Oct 29, 2024

Did you do some testing to confirm that all is now well?

@wikumChamith
Copy link
Member Author

Did you do some testing to confirm that all is now well?

Yes. I tried this on refapp 2.14 snapshot.

@dkayiwa
Copy link
Member

dkayiwa commented Oct 29, 2024

@wikumChamith can we try to get something from known and reputable organisations like Apache, etc?

@wikumChamith
Copy link
Member Author

wikumChamith commented Oct 29, 2024

an we try to get something from known and reputable organisations like Apache

Apache Commons Imaging does not support webp files. I think we could use TwelveMonekeys. It is more well-managed.

@dkayiwa
Copy link
Member

dkayiwa commented Oct 29, 2024

Not to even just load them?

@dkayiwa
Copy link
Member

dkayiwa commented Oct 29, 2024

@wikumChamith since it supports parsing and extracting metadata from the image file, i guess that is all we need to load it. We are not interested in encoding or decoding the image. Are we?

@wikumChamith
Copy link
Member Author

@wikumChamith since it supports parsing and extracting metadata from the image file, i guess that is all we need to load it. We are not interested in encoding or decoding the image. Are we?

We need to read image files to extract data, such as retrieving image dimensions.

https://github.com/openmrs/openmrs-module-attachments/blob/b0cb957295f0c737a6b973acaf4c60a43e0f7a3e/api/src/main/java/org/openmrs/module/attachments/obs/ImageAttachmentHandler.java#L83-L87

Apache commons-imaging also still does not support webp files: https://commons.apache.org/proper/commons-imaging/roadmap.html

If we're unable to use a plugin for ImageIO, we may need to wait for a full release of Apache Commons Imaging.

@dkayiwa
Copy link
Member

dkayiwa commented Oct 30, 2024

@wikumChamith have you tried out the latest alpha version of apache commons imaging to see what exactly happens? If yes, what errors do you get?

@wikumChamith
Copy link
Member Author

@wikumChamith have you tried out the latest alpha version of apache commons imaging to see what exactly happens? If yes, what errors do you get?

Yes, I tried that. I am getting an error stating that webp is still not supported.

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.

2 participants