Add WebP image conversion support #1307
Conversation
peter-gribanov
changed the title
|
@tomol1111 I tried to adhere to the coding style of this project and it does not seem that it is customary to use type hinting in this project. |
There was a problem hiding this comment.
looks good to me.
@tomol1111 I agree with @peter-gribanov that it would be odd to add the typehints for int / bool just for these new methods. it will make the code inconsistent. if somebody wants to make the effort to make everything typed, i think it would be cool - though it can be very tricky with regard to backwards compatibility as we sometimes might accept weird usage that would then stop working with the type declarations.
Ok :) I saw somewhere type hint, so added some comments. Good job :) |
Service/FilterService.php
Outdated
| @@ -97,6 +114,16 @@ public function getUrlOfFilteredImage($path, $filter, $resolver = null) | |||
| $resolver | |||
| ); | |||
|
|
|||
| if ($this->webpGenerate) { | |||
There was a problem hiding this comment.
Hello!
It assumes that the cache for the previous image has not been created, so it required to remove all previous cache in order to generate the webp format. It's not a big deal but i think it show an implementation issue.
We can build paths to resolve first then creating the cache for all the paths
public function getUrlOfFilteredImage($path, $filter, $resolver = null, $generateWebp = false)
{
return $this->generateCacheFor($path, $filter, $generateWebp, $resolver);
}
public function getUrlOfFilteredImageWithRuntimeFilters(
$path,
$filter,
array $runtimeFilters = [],
$resolver = null,
$generateWebp = false
) {
return $this->generateCacheFor($path, $filter, $generateWebp, $runtimeFilters, $resolver);
}
public function generateCacheFor($path, $filter, $generateWebp, array $runtimeFilters = [], $resolver = null)
{
$pathsToResolve = [
new PathToResolveConfiguration(
$path,
[
'filter' => $runtimeFilters,
]
)
];
// I also suggest to have only webp as file extension
if ($generateWebp === true) {
$webpPath = str_replace(
\pathinfo($path, PATHINFO_EXTENSION),
'webp',
$path
);
$pathsToResolve []= new PathToResolveConfiguration(
$webpPath,
[
'quality' => $this->webpQuality,
'format' => 'webp',
'filter' => $runtimeFilters,
]);
}
foreach ($pathsToResolve as $pathToResolve) {
if ($this->cacheManager->isStored($pathToResolve->path(), $filter, $resolver)) {
return $this->cacheManager->resolve($pathToResolve->path(), $filter, $resolver);
}
$filteredBinary = $this->createFilteredBinary(
$pathToResolve->path(),
$filter,
$pathToResolve->options()
);
$this->cacheManager->store(
$filteredBinary,
$pathToResolve->path(),
$filter,
$resolver
);
return $this->cacheManager->resolve($pathToResolve->path(), $filter, $resolver);
}
}
private function createFilteredBinary($path, $filter, array $options)
{
$binary = $this->dataManager->find($filter, $path);
try {
return $this->filterManager->applyFilter($binary, $filter, $options);
} catch (NonExistingFilterException $e) {
$message = sprintf('Could not locate filter "%s" for path "%s". Message was "%s"', $filter, $path, $e->getMessage());
$this->logger->debug($message);
throw $e;
}
}
class PathToResolveConfiguration
{
private $path;
private $options;
public function __construct($path, array $options)
{
$this->path = $path;
$this->options = $options;
}
public function path()
{
return $this->path;
}
public function options()
{
return $this->options;
}
}
But i don't like either the condition if ($generateWebp === true) in this service which is more a functionnality in certain situation, a better way can be to use the Decorator pattern over Service/FilterService which will add the behavior of build also the webp image.
This decorator will be used depending of the context, if parameter given in cli to generate also webp or in the http controller if the browser accept webp.
Can be this implementation
```php class WebpFilterServiceDecorator { /** * @var FilterService */ private $filterService;
.. delegate other public method of FilterService (Decorator pattern)
public function getUrlOfFilteredImage($path, $filter, $resolver = null)
{
$cachedPath = $this->filterService->getUrlOfFilteredImage($path, $filter, $resolver);
$this->generateCacheForWebP($path, $filter, [], $resolver);
return $cachedPath;
}
public function getUrlOfFilteredImageWithRuntimeFilters(
$path,
$filter,
array $runtimeFilters = [],
$resolver = null,
$webp = false
) {
$cachedPath = $this->filterService->getUrlOfFilteredImage($path, $filter, $resolver);
$this->generateCacheForWebP($path, $filter, $runtimeFilters, $resolver);
return $cachedPath;
}
private function generateCacheForWebP($path, $filter, $runtimeFilters, $resolver)
{
$webpPath = str_replace(
\pathinfo($path, PATHINFO_EXTENSION),
'webp',
$path
);
$options = [
'quality' => $this->webpQuality,
'format' => 'webp',
'filter' => $runtimeFilters,
];
$this->filterService->generateCacheFor($path, $filter, $options, $resolver);
}
}
class FilterService
{
...
public function getUrlOfFilteredImage($path, $filter, $resolver = null)
{
$options = [
'filters' => [],
];
return $this->generateCacheFor($path, $filter, $options, $resolver);
}
public function getUrlOfFilteredImageWithRuntimeFilters(
$path,
$filter,
array $runtimeFilters = [],
$resolver = null
) {
$options = [
'filters' => $runtimeFilters,
];
return $this->generateCacheFor($path, $filter, $options, $resolver);
}
public function generateCacheFor($path, $filter, $generateWebp, array $options, $resolver = null)
{
if ($this->cacheManager->isStored($path, $filter, $resolver)) {
return $this->cacheManager->resolve($path, $filter, $resolver);
}
$filteredBinary = $this->createFilteredBinary(
$path,
$filter,
$options
);
$this->cacheManager->store(
$filteredBinary,
$path,
$filter,
$resolver
);
return $this->cacheManager->resolve($pathToResolve->path(), $filter, $resolver);
}
private function createFilteredBinary($path, $filter, array $options = [])
{
$binary = $this->dataManager->find($filter, $path);
try {
return $this->filterManager->applyFilter($binary, $filter, $options);
} catch (NonExistingFilterException $e) {
$message = sprintf('Could not locate filter "%s" for path "%s". Message was "%s"', $filter, $path, $e->getMessage());
$this->logger->debug($message);
throw $e;
}
}
}
</p>
</details>
There was a problem hiding this comment.
A decorator imposes complexity in configuration and can increase the number of points of BC breaks.
And yes, i agree that this example is not very optimal.
Our project uses a different approach for generating WebP.
There was a problem hiding this comment.
You forgot one more condition. WebP version of the image will be returned not only if WebP generation is enabled, but the browser also supports WebP. We get this value through the method arguments, and if we make a decorator for WebP, then this argument will be useless in FilterService. That is, we cannot use the decorator pattern here.
I also suggest to have only webp as file extension
Your code may overwrite existing files.
In my example, the original file extension becomes part of the file name. I don't see any problems in adding a suffix, but there may be problems with changing the file extension.
It assumes that the cache for the previous image has not been created, so it required to remove all previous cache in order to generate the webp format.
Thanks for this comment. I will fix this problem.
|
I add documentation and add |
|
Awesome that you are working on this! My only comment is that I'm not sure if detecting the browser support should be the responsibility of this library. Implementation of webP with fallback is mostly done by using a picture element that will detect the support inside the browser. Browser support for the html picture element is already very good. All browsers that support webp also support the picture element. Wouldn't it be better to offer a webp url and a jpg url so developers can implement which image they use themself ? |
Interesting opinion. I have come across the use of liip_imagine:
filter_sets:
my_thumb:
filters:
thumbnail: { size: [223, 223], mode: inset }
my_thumb_webp:
format: webp
quality: 100
filters:
thumbnail: { size: [223, 223], mode: inset }<picture>
<source srcset="{{ '/relative/path/to/image.jpg' | imagine_filter('my_thumb_webp') }}" type="image/webp">
<source srcset="{{ '/relative/path/to/image.jpg' | imagine_filter('my_thumb') }}" type="image/jpeg">
<img src="{{ '/relative/path/to/image.jpg' | imagine_filter('my_thumb') }}" alt="Alt Text!">
</picture>My solution is optional. It allows you to enable WebP support on the project without rewriting the source code. Improving website rendering speed with minimal investment. |
It's true that its adds some extra html markup but you see implementation of picture tags a lot. But maybe not so much just for webp support. So I'm on board with providing a link that returns the best possible image. Image cloud services work the same way and probably for the same reason, easier to adopt. Anyway very exited to start using this, will make our websites a lot faster! And save the user some bandwidth. |
|
Something to think about is that in a near future all browsers support webp. At this time only safari 14 before Mac OS Big sur has no support. So if you don't want to support IE11 than only providing webp might be enough and there is no need to save a additional jpg to the server as fallback. |
In this case, you can now simply set the WebP format as a default format. liip_imagine:
default_filter_set_settings:
format: webp |
|
Hi Peter, Very happy to see this PR coming :) Liip need this feature. But with your solution I do not have the impression that it is possible to plug a post processors ? I think it would be cool to give the user the option to plug in a custom processors with cwebp to perform webp compression to improve the final size of the image. |
Indeed and MSIE Support officially ends August 2021. |
@linkrb are your concerns addressed by the improvements @peter-gribanov made? |
yes seems good to me. Thanks a lot @peter-gribanov :) |
|
great .. merged will release 2.4.0 in the coming days. thx so much. by far the most requested feature. |
I noticed the documentation in the OP already mentioned/copied in the symfony docs (https://symfony.com/doc/2.x/bundles/LiipImagineBundle/configuration.html) but it seems to not be working at the moment... Any news on the release of 2.4.0 and this "feature"? |
|
opps .. seems like I failed to release 2.4.0 .. will do it ASAP |
| ->defaultFalse() | ||
| ->validate() | ||
| ->ifTrue(function ($v) { | ||
| return !$v || function_exists('imagewebp'); |
There was a problem hiding this comment.
Shouldn't this be !function_exists('imagewebp')? See #1323
There was a problem hiding this comment.
I think too, i've created pull request #1322
There was a problem hiding this comment.
Read this as generate option is valid if generate option is disabled or enabled and WebP support by PHP.
Sorry for this. My mistake.
|
I would much rather this option to specify each webp filter. This doesn't seem to work though? I dislike that by using your webp option that we lose the ability for cached images to load directly. My preference would be to use the
|
|
I've noticed that when using this code, my "webp" images are always larger than the normal jpg. Is this conversion working correctly? Looking at the code, I can't see where it actually tells it to reformat the image? |
Why is the ability to load cached images directly lost?
You can specify different formats, but you have to duplicate filters. It would be great to configure once the list of allowed formats and the quality for them as in the example below. But such a change requires major code rework and will break BC. liip_imagine:
default_format: jpeg
formats:
jpeg:
quality: 80
cache: ~
data_loader: ~
post_processors: []
webp:
quality: 80
cache: ~
data_loader: ~
post_processors: []
avif:
quality: 80
cache: ~
data_loader: ~
post_processors: []
filter_sets:
my_thumb:
filters:
thumbnail: { size: [64, 64], mode: inset }
formats:
jpeg:
quality: 100
#cache: ~
#data_loader: ~
#post_processors: []
webp:
quality: 100
#cache: ~
#data_loader: ~
#post_processors: []
avif:
quality: 100
#cache: ~
#data_loader: ~
#post_processors: []usage <picture>
<!-- /cache/resolve/my_thumb/avif/relative/path/to/image.jpg -->
<source srcset="{{ '/relative/path/to/image.jpg' | imagine_filter('my_thumb', 'avif') }}" type="image/avif">
<!-- /cache/resolve/my_thumb/webp/relative/path/to/image.jpg -->
<source srcset="{{ '/relative/path/to/image.jpg' | imagine_filter('my_thumb', 'webp') }}" type="image/webp">
<!-- /cache/resolve/my_thumb/jpeg/relative/path/to/image.jpg -->
<source srcset="{{ '/relative/path/to/image.jpg' | imagine_filter('my_thumb', 'jpeg') }}" type="image/jpeg">
<!-- use "jpeg" as default format -->
<!-- /cache/resolve/my_thumb/jpeg/relative/path/to/image.jpg -->
<img src="{{ '/relative/path/to/image.jpg' | imagine_filter('my_thumb') }}" alt="Alt Text!">
</picture>
This is quite possible if you haven't changed the default quality. liip_imagine:
webp:
quality: 100
See this part of the code. |
Directly loading images from the source file is lost because all requests now go through the Controller Action. This is going to be bad for performance because it's going to mean more work for the server. The server needs to determine if the request should be redirected to the jpg or webp format. For example:
First request: https://domain.com/cache/resolve/foo/bar.jpg
First request: https://domain.com/media/cache/resolve/foo/bar.jpg If you have 20 images on a page, that means 1 request to the backend for the initial request and then 20 requests to the backend to determine where to redirect each request. nginx/apache is going to serve the static files much more quickly.
Creating multiple filter sets is not a problem. I don't want the magic behind this PR redirecting my images. I just want to be able to create a filter set (specifically for webp) and then use the I can even write a compiler pass to duplicate all my filtersets and just append an "_webp" to each. The documentation does not outline how webp can be configured other than by enabling the controller action above. Is this actually possible or does your PR only make webp available through the controller action?
Should it not be creating a webp from the image that has already been cropped/reduced in size by the other filter sets? Not sure how it would get bigger?
|
|
Just to add to this, I am not seeing any reduction in size even with your example config. My webp files are increasing in size. If I take the original images and run it through "cwebp" (https://developers.google.com/speed/webp/docs/using), I see a massive reduction in size (with quality as 100) |
This is a feature of the implementation of this project, and not specifically of this feature.
Then i don't understand what the problem is. Why are you not satisfied with this solution?
The documentation provides 2 ways to render WebP. We probably need to add this way to the documentation.
Typically, WebP weighs less than JPEG, but in some cases this is not.
To create and format pictures, the Imagine project is used, which uses the driver GD, ImageMagick or Gmagick of your choice. You can contact the developers of these projects to clarify the specifics of the implementation of image formatting. |
|
Thank you for the link to #1307 (comment) I was not aware that it would automatically convert it to webp just by specifying the format. I'll look further into why my images are increasing in size. |
I just had a thought on this, at the moment, you're only checking if WEBP is supported when generating the image. Why don't we just check this when resolving the image. The browser will tell us if it accepts image/webp in the original request. With this information, we could then decide which version would be rendered. That would mean we could still get the benefit of resolving images directly to the webp/non webp that is already generated. This is how it currently behaves without webp enabled. |
|
exactly this is the way to go .. but we would then have to add content type negotiation to this bundle, which is something we absolutely should for 3.x |
WebP support is checked during image creation and resolution. If the image has not yet been created, then it is created. After all the images have been prepared, we will resolve the path by checking the WebP support. See We cannot check WebP support during page rendering, so as not to send every request to the controller. |
|
I'm not sure why the new filters are required. Currently without this PR, an image will always resolved to the cached path. You are now checking for the resolved path within the Controller always if webp is enabled. Within that controller, you check if the image/webp is within the HTTP_ACCEPT header. I'm not sure why you can't just do the check within the HTTP_ACCEPT header from the root request without the need to do it within the action? |
|
I described this problem in PR #1333.
We can define support for WebP when rendering a page in templates, but in this case, you cannot use page caching in any form, because it may happen that users whose browser does not support WebP will be provided with pages with images in WebP format, that is, users will not see ALL images on the site.
|
@peter-gribanov, I have just tested the following config: liip_imagine:
filter_sets:
my_thumb_webp:
format: webp
quality: 100
filters:
thumbnail: { size: [223, 223], mode: inset }The content type returned is image/jpeg and then saving the file and running There is no image conversion happening in this instance. Should there be? |
|
Apologies, I believe apache is altering the mime type when serving the file because the extension is ".jpg". If I run a mime check on the cached file within the application, it returns image/webp I'm not too sure how to handle this one. I know there is a PR open to alter the extension so maybe I'll need to wait for that. |
This is a simple solution.
Each time a filtered image is created, a copy of it is created in the WebP format and saved to the same address with the same name and the
.webpsuffix. If the browser supports WebP, it redirects the user to the Webp version of the image, otherwise to the picture in the format in accordance with the configurations.Documentation
The WebP format better optimizes the quality and size of the compressed image compared to JPEG and PNG. Google strongly recommends using this format. You can set it as the default format for all images.
However, not all browsers support the WebP format, and for compatibility with all browsers it is recommended to return images in their original format for those browsers that do not support WebP. This means that you need to store 2 versions of the image. One in WebP format and the other in original format. Remember that this almost doubles the amount of used space on the server for storing filtered images.
If browser supports WebP, the request
https://localhost/media/cache/resolve/thumbnail_web_path/images/cats.jpegwill be redirected to
https://localhost/media/cache/thumbnail_web_path/images/cats.jpeg.webpotherwise to
https://localhost/media/cache/thumbnail_web_path/images/cats.jpeg