WebP support is broken

[fbourigault]

Given the following code:

<img src="{{ asset('DSC_3644.JPG') | imagine_filter('thumbnail') }}" alt="DSC_3644.JPG" />

When there is no cache, the src value is https://localhost:8000/media/cache/resolve/thumbnail/DSC_3644.JPG which is handled by the ImagineController and redirect to https://localhost:8000/media/cache/thumbnail/DSC_3644.JPG.webp

When the cache is warmed, the src value is https://127.0.0.1:8000/media/cache/thumbnail/DSC_3644.JPG which is not handled by the ImagineController and returns the JPEG version of the image.

I think we should not support WebP this way and consider using proper content negotiation which will require users to use the <picture> HTML element but will allow deterministic behavior and also support other formats such as AVIF (see #1314).

I didn't tested it, but this WebP implementation may break HTTP Caching.

I propose to revert #1307 and then to follow this plan:

• Move to GitHub Actions as travis-ci.com does not support webp (see [RFC] Switch to GitHub Actions #1318).

• Ensure that WebP can be used as a supported format by adding tests and strong configuration validation to ensure that when webp format is requested by the end user, the used driver also support it. This would avoid issues about webp format not working.

At this point, it's possible to do content negotiation through the use of multiple filters_set (one per format).

• Allow to use configure multiple formats per filter set to avoid filters_set duplications because of the format and add a format option to the imagine_filter twig filter.

At this point, it's possible to do content negotiation with <picture> and a single filters_set (configured with multiple formats).

• Allow server-side content-negotiation. IMHO, this should be done at the twig level. to avoid the issue I described above. A service could also be provided to ease integration with other systems (APIs ?).

At this point, it will be possible to do both client-side and server-side content negotiation with no compromise and also being open to add AVIF and any future format.

[martinbroos]

I'm still very exited to use webp with LiipImagine, but haven't tried the new implementation yet. But your cache issue makes sense.. I'm still not against using html picture element as content negotiation.

[lsmith77]

sorry. I didn’t see the issue before releasing 2.4.0.

I will look at how to proceed today.

@peter-gribanov can you maybe also have a look at the issue?

[lsmith77]

@fbourigault proper content negotiation is indeed the way to go.

that being said, the feature as is has been released since I didn't see this ticket in time (my bad).

so what if we basically deprecate #1307 and then rather work on a 3.0 which features content negotiation?

@fbourigault would you be willing to take the lead here?
@peter-gribanov what is your point of view here? would you be motivated to collaborate with @fbourigault on the proposal outlined above?

in that case we would merge #1323 and release 2.4.1 with this fix and a deprecation notice.

[peter-gribanov]

I didn't tested it, but this WebP implementation may break HTTP Caching.

The controller uses a 302 Moved Temporarily redirect, not a 301 Moved Permanently redirect. You need to use a 302 redirect instead of a 301.

liip_imagine:
    controller:
        redirect_response_code: 302

It is normal practice to use a 302 redirect to redirect to different URLs depending on conditions. Redirects with 302 code are not cached without explicitly specifying cache Cache-Control or Expires headers. According to RFC 2616, section 10.3.3 302 Found.

The requested resource resides temporarily under a different URI. Since the redirection might be altered on occasion, the client SHOULD continue to use the Request-URI for future requests. This response is only cacheable if indicated by a Cache-Control or Expires header field.

The user's browser can cache the redirect, but in the case of a specific browser, the redirect will be correct. Another browser will have a different cache and a different redirect. The problem can only be with caching proxies and only if you are not using HTTPS. HTTP caching does not work with HTTPS.

301 redirects when using a secure connection (HTTPS) must also be cached separately for each user and must be unique for each user.

• Ensure that WebP can be used as a supported format by adding tests and strong configuration validation to ensure that when webp format is requested by the end user, the used driver also support it. This would avoid issues about webp format not working.

Has already. See #1307

I think we should not support WebP this way and consider using proper content negotiation which will require users to use the HTML element but will allow deterministic behavior and also support other formats such as AVIF.

This subject has already been discussed. Implementing what you want will be difficult and will break BC. The link to the picture should contain not only the name of the filter, but also the expected format.

https://localhost:8000/media/cache/resolve/thumbnail/jpeg/DSC_3644.JPG
https://localhost:8000/media/cache/resolve/thumbnail/avif/DSC_3644.JPG
https://localhost:8000/media/cache/resolve/thumbnail/webp/DSC_3644.JPG

Do you want to receive something like this?

liip_imagine:
    filter_sets:
        thumbnail:
            formats:
                avif:
                    quality: 100
                    post_processors:
                        my_custom_avif_post_processor: { }
                webp:
                    quality: 80
                    post_processors:
                        my_custom_webp_post_processor: { }
                jpeg:
                    quality: 100
                    post_processors:
                        my_custom_jpeg_post_processor: { }
            filters:
                thumbnail: { size: [223, 223], mode: inset }

I repeat once again, the main idea of ​​my feature is to enable WebP support in the project with minimal effort, without changing the project code as much as possible. Very soon, the need for such tricks will disappear. https://caniuse.com/webp

In our project several years ago we abandoned the redirect in order to improve performance and did not face the described problems. This feature has been successfully working for us for several years.

[fbourigault]

It is normal practice to use a 302 redirect to redirect to different URLs depending on conditions. Redirects with 302 code are not cached without explicitly specifying cache Cache-Control or Expires headers. According to RFC 2616, section 10.3.3 302 Found.

This is mandatory for proper use of current WebP support and must be documented.

Has already. See #1307

It only validates against libGD but this bundle also supports imagick and gmagick. Moreover, the current check is broken. See (#1322). This is also the reason I disabled the imagewebp function in https://github.com/liip/LiipImagineBundle/blob/master/.github/workflows/ci.yml#L54 to avoid test failure in GitHub Actions (which has GD with WebP support unlike Travis CI).

This subject has already been discussed. Implementing what you want will be difficult and will break BC. The link to the picture should contain not only the name of the filter, but also the expected format.

I repeat once again, the main idea of ​​my feature is to enable WebP support in the project with minimal effort, without changing the project code as much as possible. Very soon, the need for such tricks will disappear. https://caniuse.com/webp

The idea I came with is far for perfect and must be battle tested. Until WebP came out, the web only known about jpeg/png/gif/bmp. Now there is WebP which is has ~ 90% support. Avif is coming (https://caniuse.com/avif) and tomorrow there will be an other yet to be invented format. What I would also implement here is to build foundations to make this bundle future proof to reduce the maintenance burden and ease implementation of new formats.

Given the following code:

<img src="{{ asset('DSC_3644.JPG') | imagine_filter('thumbnail') }}" alt="DSC_3644.JPG" />

When there is no cache, the src value is https://localhost:8000/media/cache/resolve/thumbnail/DSC_3644.JPG which is handled by the ImagineController and redirect to https://localhost:8000/media/cache/thumbnail/DSC_3644.JPG.webp

When the cache is warmed, the src value is https://127.0.0.1:8000/media/cache/thumbnail/DSC_3644.JPG which is not handled by the ImagineController and returns the JPEG version of the image.

This is still the main reason why I think current WebP implementation in this bundle is broken.

I will try to put this in a test or push a small reproducer.

In our project several years ago we abandoned the redirect in order to improve performance and did not face the described problems. This feature has been successfully working for us for several years.

I understand why you implemented WebP support like this, but I think this bundle should let the user decide if the content negotiation should be done on client side or server side, globally or per filter.

[peter-gribanov]

It only validates against libGD but this bundle also supports imagick and gmagick. Moreover, the current check is broken.

I fix this problem #1326

I understand why you implemented WebP support like this, but I think this bundle should let the user decide if the content negotiation should be done on client side or server side, globally or per filter.

The more choice the user has, the better for him, but also the worse for this bundle, since all these conditions must be crammed into one controller. This can make the bundle setup very difficult.

[fbourigault]

I looked (again) at the current WebP implementation and there are the points that I think should be addressed:

1. Fix configuration validation. Fix #1322 configuration webp validate #1323 is the way to go to allow webp with any driver.
2. Document to not use 301/308 redirects when webp support is enabled.
3. Fix the twig/templating imagine_filter to also do the content negotiation.

When those 3 points will be addressed, I think this issue will be resolved.

After I think we should define what we want for a 3.0 and start deprecating things to get this bundle internals simpler.

[peter-gribanov]

Fix configuration validation. #1323 is the way to go to allow webp with any driver.

Did you mean #1326?

Fix the twig/templating imagine_filter to also do the content negotiation.

I didn't understand this point. Clarify please.

[fbourigault]

Did you mean #1326?

Yes 😅

I didn't understand this point. Clarify please.

That's what I wrote at the beginning of this issue:

Given the following code:

<img src="{{ asset('DSC_3644.JPG') | imagine_filter('thumbnail') }}" alt="DSC_3644.JPG" />

When there is no cache, the src value is https://localhost:8000/media/cache/resolve/thumbnail/DSC_3644.JPG which is handled by the ImagineController and redirect to https://localhost:8000/media/cache/thumbnail/DSC_3644.JPG.webp

When the cache is warmed, the src value is https://127.0.0.1:8000/media/cache/thumbnail/DSC_3644.JPG which is not handled by the ImagineController and returns the JPEG version of the image.

[peter-gribanov]

Given the following code:

<img src="{{ asset('DSC_3644.JPG') | imagine_filter('thumbnail') }}" alt="DSC_3644.JPG" />

When there is no cache, the src value is https://localhost:8000/media/cache/resolve/thumbnail/DSC_3644.JPG which is handled by the ImagineController and redirect to https://localhost:8000/media/cache/thumbnail/DSC_3644.JPG.webp

When the cache is warmed, the src value is https://127.0.0.1:8000/media/cache/thumbnail/DSC_3644.JPG which is not handled by the ImagineController and returns the JPEG version of the image.

Sorry, but i see no problem with filter imagine_filter. This is more like the HTTP caching problem discussed earlier.

[fbourigault]

When the cache is warmed, the src attribute value is https://127.0.0.1:8000/media/cache/thumbnail/DSC_3644.JPG but not https://127.0.0.1:8000/media/cache/thumbnail/DSC_3644.JPG.webp so any client get served the jpeg even when webp is supported.

[peter-gribanov]

This is strange. The WebP version should be returned. Perhaps the problem is related to your web server settings?

[fbourigault]

The web server is not involved. This is pure twig. I will push a reproducer asap.

[peter-gribanov]

The web server is not involved. This is pure twig. I will push a reproducer asap.

Now i understand what you are talking about. The bundle resolves the path to Twig every time, and if the file exists, it generates a link to it, instead of always sending it to the controller to resolve the path. I already forgot that we also optimized this part of the code. Fix this in #1333.

[peter-gribanov]

Document to not use 301/308 redirects when webp support is enabled.

Add this in docs #1334

[Amunak]

I'd like to add that we should be able to configure when WebP is to be used/redirected. I have extremely-well optimized options for thumbnails, where the JPEG is about 23KB and the same WebP is 240KB. And since I can't tweak WebP the same way, it'll always be bigger.

But I would still like to use WebP for the larger, non-thumbnail images, where a full-quality JPEG is 190KB and WebP is 90KB.

[dbu]

i think the things discussed here have been fixed now.

if somebody wants to make a pull request to allow enabling/disabling webp per filter instead of globally, you are welcome to do that.