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

Add webp and avif conversion to strapi-plugin-upload #47

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Antoine-lb
Copy link

@strapi-cla
Copy link

strapi-cla commented Aug 16, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

I really like the intention.

I'm wondering if we should allow some more configurations for this. Let's say you want to only convert png to webp and not jpg for some reason 🤔

Something that I see might be a request would be to make this dynamic so you can decide at runtime which conversions to run. Would be nice to think about that in the rfc

export default ({ env }) => ({
upload: {
config: {
convertAllImagesTo: 'webp' | 'avif'; // To be defined
Copy link
Member

Choose a reason for hiding this comment

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

Do you have some resource one which conversions are supported to webp? is it jpg/png to webp or are there more formats supported?

How does sharp handle non compatible formats ? is it just not doing the transform or throws ? if so we would need to have a mapping to maintain?

@Antoine-lb
Copy link
Author

Antoine-lb commented Aug 18, 2022

Do you have some resource one which conversions are supported to webp?

From the documentation (https://sharp.pixelplumbing.com/#formats):

  • can be input : JPEG, PNG, WebP, GIF, AVIF, TIFF, and SVG images.
  • can be output:JPEG, PNG, WebP, GIF, AVIF, TIFF, and uncompressed raw pixel data.

How does sharp handle non compatible formats ?

From my small non exhaustive testing:

  • Reading a non supported format sharp threw an error: heif: Unsupported feature: Unsupported codec (4.3000)
  • Outputting to a non-supported format didn’t throw an error, but it just renamed the file (e.g dog.jpeg was converted to dog.doesnotexist, but MacOS still recognizes it as a JPEG image)

Important note: Converting a large .jpeg to a .png caused the image to go from 2.7MB to 32.2MB ⚠️ (whereas when converted to .webp it went to 1.4MB). The test was done in just 1 picture. I will do further testing on the side effects that converting can have.

if so we would need to have a mapping to maintain?

You are dealing with a noob here 😅, I didn’t get what you mean

Can it be done dynamically

I guess, technically nothing is stopping it. I would double check security-wise if it is safe (I don’t know much about security, but it looks like vulnerabilities could get through)

Let's say you want to only convert png to webp and not jpg for some reason

At a first look, this could also be done.


  • I will do more testing on converting files, to see if is even a good idea to enable them (if going from jpeg to png always increases the size of the file it may not be a good idea to enable it)

@Antoine-lb
Copy link
Author

Antoine-lb commented Aug 18, 2022

Personal opinion and motivations
I’m building a product with Strapi where multiple users can upload images. Users and I don’t care about the image format as long as it looks good and that it loads properly. If Google tells me that I should used WebP, I have no good argument for not doing it.
So:

  • If it can be done dynamically: I will block it, I don’t want my users to choose that, they have no reason to do so, it would be weird (malicious) if one of them tries to do it.
  • More granular approach on what formats to convert (e.g. only convert .png): I would not use it either, and I’m having a hard time finding a good reason for someone to do that

I see this feature for people that don't want to think about images instead of people who want more granular control.

A more "batteries included" approach. Very opinionated, but is most likely the best option for you.

Question:

  • How do you proceed with the product-oriented questions? is there a standard way of proceeding?

@knulpi
Copy link

knulpi commented Jan 23, 2023

I'm wondering if we should allow some more configurations for this. Let's say you want to only convert png to webp and not jpg for some reason 🤔

This would be valid then also for other image formats, right? So e.g. transforming an uploaded PNG into an JPG and WebP, or transforming an uploaded WebP into an AVIV (just a made up example). While I do not really see a use case...what do you think about a configurable array here? I would suggest something like:

conversions: [
  {
    from: ["PNG", "JPG"]
    to: ["WebP", "TIFF", "AVIV"]
    keepSourceFile: true // will keep (and resize (if applicable)) originally uploaded file
  },
  {
    from: ["*"],
    to: ["AVIV"],
    keepSourceFile: false // will delete and not resize (if applicable) the original file 
  }
]

Something that I see might be a request would be to make this dynamic so you can decide at runtime which conversions to run. Would be nice to think about that in the rfc

Looking at the current configuration mechanism of the upload plugin (being statically configured in the plugins.js) I do not really see why it should be dynamic here. I agree though, that it might be a nice feature for the future. What about taking the static configuration for a first shot and thinking about an admin-controlled configuration mechanism as outlook? (admin-controlled, since I share some of the concerns @Antoine-lb brought up...)

@Antoine-lb
Copy link
Author

I didn't have time to work on this, but the main blocker right now is whether it is a good idea to change the format altogether.

What I mean by that is that these algorithms are clearly good when you go from a raw or uncompressed image to a compressed image. But when you are dealing with already compressed images, things can go wrong.

I did some testing with sharp (the lib used by Strapi) and when going from a reasonably sized jpeg  to webp, the image actually got bigger, whereas just optimizing the jpeg got me great results.

And when converting jpegs to pngs they almost always got bigger, which makes sense for jpeg and webp.

So basically just transforming all images to webp may not be the best idea in all cases.

I did very little test and research, and I know very little about image compression, so take this very lightly.

Current hypothesis:

  • Adding some hard coded rules to avoid images getting bigger may be required (for example, not processing reasonably sized jpegs)
  • Maybe just transform some types and not others (pngs are doing well when converted to webp, and webp supports transparency)
  • Maybe is not that big of an issue altogether, and is ok to transform all of them to webp
  • Maybe all of this is a terrible idea

If I'm totally honest, we lost interest in building this because now we are converting all images to webp manually ourselves. But I still believe this is a feature Strapi should consider, specially for the fact that sharp already allows it, so it does not require much work.

Pin me if you still want me to work on it.

@strapi-bot
Copy link

This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/image-file-format-webm-not-allowed-in-media-library-upload/34346/2

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.

None yet

5 participants