Usage of consistent webP Format

[wakabayashi]

I am working on my new theme and webP support is a big deal of it. Since we have now over 30k products data size is kind of an issue.

Right now, when you enable webp. The original image is saved as jpg. I did change this behaviour to webp. So when I upload I only have webp files. But this leads to issues in BO -> jpg is hardcoded there.

Why does actually the theme need to support webp? Is it not kind of the server, that needs to support it? Cause my idea was now: give the merchant a simple select, which formats he wants to use and then this is used for all uploaded images in FO and BO.

What do you think?

[datakick]

I never understood why the theme have to mark if it is compatible with webp or not. I suspect this is because sometimes javascript makes some weird operations. I have seen js to implement zoom functionality by replacing the 'home' image type with bigger pictures (yes, manually constructing product image url). This kind of javascript code could break if the source image was webp. But, generally speaking, there is really no reason to defer decision whether to use webp or not to theme.

I agree we could have some settings option to choose default image type. 

As you noted -- there are couple of places in the core where jpg are hardcoded. We would have to find them all, and replace them with proper way to generate image url. That would make the codebase much cleaner.

Unfortunately, there are also a lot of modules that constructs image urls manually, forces jpg, etc... for these, we should have a fallback mechanism in place. NotFound controller could probably just return webp image content with correct Content-Type, and browser would understand that.

 

[e-com]

4 hours ago, wakabayashi said:

What do you think?

I support this idea as much as possible.
With modern browsers, keeping this dual jpg+webp support for archaic browsers is pointless.
Unnecessary generation of terabytes of images to server disk 🙂
 

[wakabayashi]

Ok, nice to see that opinions are aligned. Right now, I don't know enough about the whole mechanism and it seems to be quite a huge rewrite. But I can imagine, that I will submit something in the future.

With webp and some cleanups on image formats I could save more than 50% of the filesize. 😳 But I still need to figure out, what concequences it has to the BO.

[wakabayashi]

Tonight I had the brilliant (not) idea, to look a bit into it. Looks like quite a huge project. But something confused me 🤨

@datakick Are you already working on a consistent image/webp support? I found this: https://github.com/thirtybees/thirtybees/commit/6334306227ee5df399d4b1576d536fc4c3457dec Isn't this the fallback mechanism you were talking about?

[datakick]

8 hours ago, wakabayashi said:

Tonight I had the brilliant (not) idea, to look a bit into it. Looks like quite a huge project. But something confused me 🤨

@datakick Are you already working on a consistent image/webp support? I found this: https://github.com/thirtybees/thirtybees/commit/6334306227ee5df399d4b1576d536fc4c3457dec Isn't this the fallback mechanism you were talking about?

Yes, that is the fallback mechanism, but that works for front office only. We will probably have to extend it to support back office image links as well -- those image urls can be relative, does not contain image types, etc. Maybe it will work without changes, I'm not sure.

[wakabayashi]

Project of consistent webp support (and image cleanup in general) started: https://github.com/eschiendorfer/thirtybees/tree/cleanWebpSupport

Help by another dev is welcomed 😅

[wakabayashi]

I plan to really simplify things (get rid of some code). Some questions arise in this context...

The idea is, that you select one image format and that all generated files (thumbnails) are saved in this format.

1. Should it be possible to hold original files in the original file format? Let's say you use webp, but upload a png. Should the original file always be converted in webp or should it be possible to hold it as png? The later makes things more complex and bit slower.
2. Which formats should we allow to upload in BO? jpg, gif, png and webp?
3. Which formats should be allowed for thumbnails. jpg and webp for sure, but png as well? 

Edit: Also I hope that we can get a more consistent wording. The word "type" is used for at least 3 different things. It can mean entityType (product), imageType (cart_default) or imageExtension (jpg). It makes hard, to read the code like this.

Edited February 3, 2023 by wakabayashi

[the.rampage.rado]

What about modules that look for jpg? I have some part of Warehouse (mega menu and content creator) that call for jpg despite webp is enabled. What about them?

Up until now TB keeps the original source file and use it for future use? What about your idea?

I personally don't upload gifs but I imagine some product schemes may come in it so I believe they all should be enabled for upload in BO.

[wakabayashi]

15 minutes ago, the.rampage.rado said:

What about modules that look for jpg?

That's what the fallback mechanism is good for.

 

16 minutes ago, the.rampage.rado said:

Up until now TB keeps the original source file and use it for future use? What about your idea?

Sure I will hold them too. The question is in which Extension we should hold them. At the moment it's forced to be always jpg. I don't like that. I guess it's best to hold the format, that was uploaded.

[wakabayashi]

@the.rampage.rado Btw can you tell me more about the Warehouse usage of jpg? How and where are this images be loaded? Is this in a tpl file and '.jpg' is hardcoded?

[the.rampage.rado]

This is what's hardcoded in both modules.

Now I notice a strange behaviour - sometimes the modules load webp and sometimes they load jpg. What could be causing this? I mean same image, same place in the module when called from different categories/products displays different format.

[wakabayashi]

@datakick I am bit surprised how many function we have about getting ImageTypes and Image Link. It seems to me, that this is often about guessing, how an image type could be named. Multiple times we even search for something like '_default'. Why is this needed?

I had the idea to use ImageTypes in a different way. There are a few thoughts:

• A theme designer always knows the image_type name (he has no problem to generate a link)
• A core dev and module dev don't know the images types for sure. (they might have a problem)
• BUT: always when you want to get an image_type it's actually about sizing (correct me if this is wrong?). 

So why don't we create a function getImageTypeBySize($min_height, $min_width), that does return the best existing/matching image_type. Of course we could then even improve this by something getImageLinkBySize(). Couldn't we drop then all the "guessing" like:

• Link::getProductImageUri()
• Link::getProductDefaultImageUri()
• ImageType::resolveImageTypeNameWithoutCache()

These things are actually not totally related to image extension, but it's quite complicated to cleanup files at some points. It's very hard to forseen, what happens if you replace a hardcoded '.jpg' with something like $imageExtension that could be everything.

Edited March 28, 2023 by wakabayashi

[datakick]

15 hours ago, wakabayashi said:

@datakick I am bit surprised how many function we have about getting ImageTypes and Image Link. It seems to me, that this is often about guessing, how an image type could be named. Multiple times we even search for something like '_default'. Why is this needed?

Compatibility reasons. Image type name should always be provided without theme prefixes, for example 'home' or 'cart'. However, sometimes module developers used 'home_default' instead of 'home', where 'default' is name of ps16 default theme 🙂  Such module worked correctly on default theme only.

Then some theme developer copied default theme, but kept the image type names 'home_default', which ultimately lead to formatted name like 'Niara_home_default'.

So yeah, there is a lot of fixing and adjusting, otherwise most module would stop working.

This test summarize this mess quite nicely: https://github.com/thirtybees/thirtybees/blob/main/tests/Unit/ImageTypeTest.php 

15 hours ago, wakabayashi said:

I had the idea to use ImageTypes in a different way. There are a view thoughts:

• A theme designer always knows the image_type name (he has no problem to generate a link)
• A core dev and module dev don't know the images types for sure. (they might have a problem)
• BUT: always when you want to get an image_type it's actually about sizing (correct me if this is wrong?). 

So why don't we create a function getImageTypeBySize($min_height, $min_width), that does return the best existing/matching image_type. Of course we could then even improve this by something getImageLinkBySize(). Couldn't we drop then all the "guessing" like:

• Link::getProductImageUri()
• Link::getProductDefaultImageUri()
• ImageType::resolveImageTypeNameWithoutCache()

These things are actually not totally related to image extension, but it's quite complicated to cleanup files at some points. It's very hard to forseen, what happens if you replace a hardcoded '.jpg' with something like $imageExtension that could be everything.

I don't think that module developers need specific sizes that often. What they want is their module to integrate seamlessly into the theme design. To do that, they should not make any assumptions about image sizes in the first place. For example, there can be a theme that displays very large product images on home page. Instead of standard 250x250, this theme renders only few products on home page with 500x500. Any module that want to display on home page some additional products should probably render it in the same size, otherwise it would look bad. If module developer used here getImageTypeBySize(250, 250) the result would not be nice. 

I think that image type abstraction is not a bad thing. What is wrong is hardcoding image types into templates. These should be configurable. Nowhere in the templates we should see strings constants.

I agree that it would be very nice addition to have a method to choose image type by size. Sometimes it is indeed needed. 

 

[wakabayashi]

6 hours ago, datakick said:

To do that, they should not make any assumptions about image sizes in the first place.

Ok, I have never thought that way. But yeah it makes sense to me too (at least in theory). So if this is true, it bascially means, that a module dev should "never" width="" height="" in a tpl file, right? But imo opinion it needs to be clear, what kind of type a module dev needs to load (in every situation). I guess that is what you mean by "Nowhere in the templates we should see strings constants."

6 hours ago, datakick said:

Compatibility reasons.

Haha, actually as almost always, when we have a lot of code for a single functionality 😅

Ok, anyway I will try to continue my rewrite as good as possible. But this PR will be huge (lots of files invovled) and serious checks will be needed. I see more and more the need of this: https://github.com/thirtybees/thirtybees/pull/938. I believe, it's almost impossible, to have always a 100% clean image folder, that is also performant.

[datakick]

16 hours ago, wakabayashi said:

I guess that is what you mean by "Nowhere in the templates we should see strings constants."

I meant that modules should not use hardcoded string 'home' or 'cart' like this

<img src="{$link->getImageLink($rewrite, $imageId, 'home')}">

Because 'home' image type does not have to exists.

Modules should allow user to select image type they want to use in configuration page, and pass this selection to template. 

<img src="{$link->getImageLink($rewrite, $imageId, $selectedImageType)}">

Unfortunately, a lot of module developers don't do that (some of my own modules are using hardcoded image types as well), and that can cause troubles. And the need for this 'guessing' type detection.

[wakabayashi]

Damn my brain seems to be damaged 🤕 Yeah, that's (obviously) the best solution. 

The hardcoding is just working by "luck". A theme could not even have something like 'home', 'home_default', 'niara_home' or whatever. That's actually where my idea ImageTypeBySize() came from. But you are right: if module devs give a select option, actually there would be no problem at all.

Edited February 7, 2023 by wakabayashi

[e-com]

Currently, it is not possible to add and scale WebP images from within modules.
To eliminate this I use overrides of several methods in ImageManager class:
https://github.com/e-com-presta/tboverride/blob/main/ImageManager.php
Maybe something will be useful from these solutions 🙂

 

[wakabayashi]

@datakick What is something like this good for: https://github.com/thirtybees/thirtybees/blob/main/classes/Language.php#L629-L643? 

1. English language might not even be installed and in my installation there are no such files
2. Why do we need this files in all folders (category, manufacturer and so on)? Wouldn't it make more sense, to just store them in folder language ('l')?

I have seen multiple such similair usecases. 🙃

 

@e-com Thanks for your link. I will look into it, but right now I am a bit overhelmed with all other things related to this rewrite ^^ Could you shortly explain what/why is not working with webp? Cause we have a webp support. I know, that it's not possible to upload webp images in BO for products. But that is actually just because of hardcoded values in some controller. In general I believe, that there is no issue with generating webp images... Might be wrong of course 🥵 As you are very active and experienced dev, will you help to check my rewrite once I "finished" it? There will be bugs or at least stuff, I haven't taken into account. It's not possible for me to forseen/test every detail. 🙈

[datakick]

4 minutes ago, wakabayashi said:

@datakick What is something like this good for: https://github.com/thirtybees/thirtybees/blob/main/classes/Language.php#L629-L643? 

1. English language might not even be installed and in my installation there are no such files
2. Why do we need this files in all folders (category, manufacturer and so on)? Wouldn't it make more sense, to just store them in folder language ('l')?

I have seen multiple such similair usecases. 🙃

It's a actually a bug that you don't have these images 🙂

Compare result (with and without)

After installation, these images actually exists. But over the time they are lost, which is a bug

https://github.com/thirtybees/thirtybees/issues/1503

 

 

  

[wakabayashi]

52 minutes ago, datakick said:

It's a actually a bug that you don't have these images 🙂

Compare result (with and without)

After installation, these images actually exists. But over the time they are lost, which is a bug

https://github.com/thirtybees/thirtybees/issues/1503

Oh ok. Didn't know that this is an existing bug. I personally could live with the simple solution (bottom screenshot). But ok, it's not popular to remove any existing feature, so it's better to fix this cleanly.

[e-com]

@wakabayashi Already basic validateUpload() method prevents the addition of WebP images in modules. For example, in a simple module with one image "blockbanner".
I have a few test installations of TB on localhost, then of course that I will help in testing.

 

[wakabayashi]

1 hour ago, e-com said:

@wakabayashi Already basic validateUpload() method prevents the addition of WebP images in modules. For example, in a simple module with one image "blockbanner".
I have a few test installations of TB on localhost, then of course that I will help in testing.

 

Right. As you correctly noticed, we have to change isCorrectImageFileExt() a bit, so that 'webp' is accepted there 🙂

Thats why I introduced a function getAllowedImageExtensions(). At the moment is quite raw, but in general this function should return the image types that are allowed to upload. Plan is to replace all hardcoded stuff with this function. So that a extension is always (or never) supported.

Edited February 7, 2023 by wakabayashi

[wakabayashi]

@datakick What is your opinion about this copy method: https://github.com/thirtybees/thirtybees/blob/main/install-dev/models/install.php#L567-L572

1. I am concerned as we just save a png like it would be a jpg. Browsers can probably handle that, but I think it's not very good.
2. What would be the correct way to really change $imageExtension in TB? Obviously ImageManager::resize() could do this, but as no resize is involved, I dunno, if it's good to use or if we should make a new (simpler) function for it?

If I understand correct, it would be possible With Image::create() and Image::write() afterwards.

[wakabayashi]

Still working day and night on this. Always find new stuff, that looks messy and can be simplified. 

Atm I have created a system, that even Product Images can be generated by Link::getGenericImageLink(). Image urls look like:

• /products/17-Niara_large/tea-cans2x.webp
• /categories/4-Niara_category/Coffee2x.webp
• /categories/thumb/4-Niara_medium/Coffee2x.webp

Tests will show, if this is a bad idea or if its great to reduce code complexity. I believe, that way we will gain some new possibilities:

• We can quite easily give the merchants rewrite options for image urls
• In the long run we can (probably) add new image entites. This means that a module could use the core to generate thumbnails as well.

But now something happened to me, which I need some feedback. Are retina images currently working? I don't use this setting on my live store, but played with it right now. Please check out the following to links:

1. https://thirtybees.genzo.ch/products/19-Niara_thickbox/tea-cans.webp
2. https://thirtybees.genzo.ch/products/19-Niara_thickbox/tea-cans2x.webp 

It basically adds just white space around the image (probably due to a not big enough source image). Does the current system work like this as well? Looks wrong to me, but I am not totally sure.

Edited February 16, 2023 by wakabayashi

[e-com]

This is how the scaling of thumbnails has always worked both in PS 1.6 and in TB.
If source image is smaller than the generated thumbnail, a background is added around it. For JPG white, for PNG and WebP transparent.
And this is certainly correct action. By resizing images up (enlarging them) we will have ugly blurry images.